Skip to content

Commit 9f96827

Browse files
tglsfdctglsfdc
committed
Invent "amadjustmembers" AM method for validating opclass members.
This allows AM-specific knowledge to be applied during creation of pg_amop and pg_amproc entries. Specifically, the AM knows better than core code which entries to consider as required or optional. Giving the latter entries the appropriate sort of dependency allows them to be dropped without taking out the whole opclass or opfamily; which is something we'd like to have to correct obsolescent entries in extensions. This callback also opens the door to performing AM-specific validity checks during opclass creation, rather than hoping than an opclass developer will remember to test with "amvalidate". For the most part I've not actually added any such checks yet; that can happen in a follow-on patch. (Note that we shouldn't remove any tests from "amvalidate", as those are still needed to cross-check manually constructed entries in the initdb data. So adding tests to "amadjustmembers" will be somewhat duplicative, but it seems like a good idea anyway.) Patch by me, reviewed by Alexander Korotkov, Hamid Akhtar, and Anastasia Lubennikova. Discussion: https://postgr.es/m/4578.1565195302@sss.pgh.pa.us
1 parent e2b37d9 commit 9f96827

File tree

24 files changed

+646
-112
lines changed

24 files changed

+646
-112
lines changed

‎contrib/bloom/blutils.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ blhandler(PG_FUNCTION_ARGS)
139139
amroutine->amproperty = NULL;
140140
amroutine->ambuildphasename = NULL;
141141
amroutine->amvalidate = blvalidate;
142+
amroutine->amadjustmembers = NULL;
142143
amroutine->ambeginscan = blbeginscan;
143144
amroutine->amrescan = blrescan;
144145
amroutine->amgettuple = NULL;

‎doc/src/sgml/indexam.sgml

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,6 +143,7 @@ typedef struct IndexAmRoutine
143143
amproperty_function amproperty; /* can be NULL */
144144
ambuildphasename_function ambuildphasename; /* can be NULL */
145145
amvalidate_function amvalidate;
146+
amadjustmembers_function amadjustmembers; /* can be NULL */
146147
ambeginscan_function ambeginscan;
147148
amrescan_function amrescan;
148149
amgettuple_function amgettuple; /* can be NULL */
@@ -502,7 +503,48 @@ amvalidate (Oid opclassoid);
502503
the access method can reasonably do that. For example, this might include
503504
testing that all required support functions are provided.
504505
The <function>amvalidate</function> function must return false if the opclass is
505-
invalid. Problems should be reported with <function>ereport</function> messages.
506+
invalid. Problems should be reported with <function>ereport</function>
507+
messages, typically at <literal>INFO</literal> level.
508+
</para>
509+
510+
<para>
511+
<programlisting>
512+
void
513+
amadjustmembers (Oid opfamilyoid,
514+
Oid opclassoid,
515+
List *operators,
516+
List *functions);
517+
</programlisting>
518+
Validate proposed new operator and function members of an operator family,
519+
so far as the access method can reasonably do that, and set their
520+
dependency types if the default is not satisfactory. This is called
521+
during <command>CREATE OPERATOR CLASS</command> and during
522+
<command>ALTER OPERATOR FAMILY ADD</command>; in the latter
523+
case <parameter>opclassoid</parameter> is <literal>InvalidOid</literal>.
524+
The <type>List</type> arguments are lists
525+
of <structname>OpFamilyMember</structname> structs, as defined
526+
in <filename>amapi.h</filename>.
527+
528+
Tests done by this function will typically be a subset of those
529+
performed by <function>amvalidate</function>,
530+
since <function>amadjustmembers</function> cannot assume that it is
531+
seeing a complete set of members. For example, it would be reasonable
532+
to check the signature of a support function, but not to check whether
533+
all required support functions are provided. Any problems can be
534+
reported by throwing an error.
535+
536+
The dependency-related fields of
537+
the <structname>OpFamilyMember</structname> structs are initialized by
538+
the core code to create hard dependencies on the opclass if this
539+
is <command>CREATE OPERATOR CLASS</command>, or soft dependencies on the
540+
opfamily if this is <command>ALTER OPERATOR FAMILY ADD</command>.
541+
<function>amadjustmembers</function> can adjust these fields if some other
542+
behavior is more appropriate. For example, GIN, GiST, and SP-GiST
543+
always set operator members to have soft dependencies on the opfamily,
544+
since the connection between an operator and an opclass is relatively
545+
weak in these index types; so it is reasonable to allow operator members
546+
to be added and removed freely. Optional support functions are typically
547+
also given soft dependencies, so that they can be removed if necessary.
506548
</para>
507549

508550

‎src/backend/access/brin/brin.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,7 @@ brinhandler(PG_FUNCTION_ARGS)
120120
amroutine->amproperty = NULL;
121121
amroutine->ambuildphasename = NULL;
122122
amroutine->amvalidate = brinvalidate;
123+
amroutine->amadjustmembers = NULL;
123124
amroutine->ambeginscan = brinbeginscan;
124125
amroutine->amrescan = brinrescan;
125126
amroutine->amgettuple = NULL;

‎src/backend/access/gin/ginutil.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,7 @@ ginhandler(PG_FUNCTION_ARGS)
7171
amroutine->amproperty = NULL;
7272
amroutine->ambuildphasename = NULL;
7373
amroutine->amvalidate = ginvalidate;
74+
amroutine->amadjustmembers = ginadjustmembers;
7475
amroutine->ambeginscan = ginbeginscan;
7576
amroutine->amrescan = ginrescan;
7677
amroutine->amgettuple = NULL;

‎src/backend/access/gin/ginvalidate.c

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,3 +271,68 @@ ginvalidate(Oid opclassoid)
271271

272272
return result;
273273
}
274+
275+
/*
276+
* Prechecking function for adding operators/functions to a GIN opfamily.
277+
*/
278+
void
279+
ginadjustmembers(Oid opfamilyoid,
280+
Oid opclassoid,
281+
List *operators,
282+
List *functions)
283+
{
284+
ListCell *lc;
285+
286+
/*
287+
* Operator members of a GIN opfamily should never have hard dependencies,
288+
* since their connection to the opfamily depends only on what the support
289+
* functions think, and that can be altered. For consistency, we make all
290+
* soft dependencies point to the opfamily, though a soft dependency on
291+
* the opclass would work as well in the CREATE OPERATOR CLASS case.
292+
*/
293+
foreach(lc, operators)
294+
{
295+
OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
296+
297+
op->ref_is_hard = false;
298+
op->ref_is_family = true;
299+
op->refobjid = opfamilyoid;
300+
}
301+
302+
/*
303+
* Required support functions should have hard dependencies. Preferably
304+
* those are just dependencies on the opclass, but if we're in ALTER
305+
* OPERATOR FAMILY, we leave the dependency pointing at the whole
306+
* opfamily. (Given that GIN opclasses generally don't share opfamilies,
307+
* it seems unlikely to be worth working harder.)
308+
*/
309+
foreach(lc, functions)
310+
{
311+
OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
312+
313+
switch (op->number)
314+
{
315+
case GIN_EXTRACTVALUE_PROC:
316+
case GIN_EXTRACTQUERY_PROC:
317+
/* Required support function */
318+
op->ref_is_hard = true;
319+
break;
320+
case GIN_COMPARE_PROC:
321+
case GIN_CONSISTENT_PROC:
322+
case GIN_COMPARE_PARTIAL_PROC:
323+
case GIN_TRICONSISTENT_PROC:
324+
case GIN_OPTIONS_PROC:
325+
/* Optional, so force it to be a soft family dependency */
326+
op->ref_is_hard = false;
327+
op->ref_is_family = true;
328+
op->refobjid = opfamilyoid;
329+
break;
330+
default:
331+
ereport(ERROR,
332+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
333+
errmsg("support function number %d is invalid for access method %s",
334+
op->number, "gin")));
335+
break;
336+
}
337+
}
338+
}

‎src/backend/access/gist/gist.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,7 @@ gisthandler(PG_FUNCTION_ARGS)
9292
amroutine->amproperty = gistproperty;
9393
amroutine->ambuildphasename = NULL;
9494
amroutine->amvalidate = gistvalidate;
95+
amroutine->amadjustmembers = gistadjustmembers;
9596
amroutine->ambeginscan = gistbeginscan;
9697
amroutine->amrescan = gistrescan;
9798
amroutine->amgettuple = gistgettuple;

‎src/backend/access/gist/gistvalidate.c

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,3 +279,72 @@ gistvalidate(Oid opclassoid)
279279

280280
return result;
281281
}
282+
283+
/*
284+
* Prechecking function for adding operators/functions to a GiST opfamily.
285+
*/
286+
void
287+
gistadjustmembers(Oid opfamilyoid,
288+
Oid opclassoid,
289+
List *operators,
290+
List *functions)
291+
{
292+
ListCell *lc;
293+
294+
/*
295+
* Operator members of a GiST opfamily should never have hard
296+
* dependencies, since their connection to the opfamily depends only on
297+
* what the support functions think, and that can be altered. For
298+
* consistency, we make all soft dependencies point to the opfamily,
299+
* though a soft dependency on the opclass would work as well in the
300+
* CREATE OPERATOR CLASS case.
301+
*/
302+
foreach(lc, operators)
303+
{
304+
OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
305+
306+
op->ref_is_hard = false;
307+
op->ref_is_family = true;
308+
op->refobjid = opfamilyoid;
309+
}
310+
311+
/*
312+
* Required support functions should have hard dependencies. Preferably
313+
* those are just dependencies on the opclass, but if we're in ALTER
314+
* OPERATOR FAMILY, we leave the dependency pointing at the whole
315+
* opfamily. (Given that GiST opclasses generally don't share opfamilies,
316+
* it seems unlikely to be worth working harder.)
317+
*/
318+
foreach(lc, functions)
319+
{
320+
OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
321+
322+
switch (op->number)
323+
{
324+
case GIST_CONSISTENT_PROC:
325+
case GIST_UNION_PROC:
326+
case GIST_PENALTY_PROC:
327+
case GIST_PICKSPLIT_PROC:
328+
case GIST_EQUAL_PROC:
329+
/* Required support function */
330+
op->ref_is_hard = true;
331+
break;
332+
case GIST_COMPRESS_PROC:
333+
case GIST_DECOMPRESS_PROC:
334+
case GIST_DISTANCE_PROC:
335+
case GIST_FETCH_PROC:
336+
case GIST_OPTIONS_PROC:
337+
/* Optional, so force it to be a soft family dependency */
338+
op->ref_is_hard = false;
339+
op->ref_is_family = true;
340+
op->refobjid = opfamilyoid;
341+
break;
342+
default:
343+
ereport(ERROR,
344+
(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
345+
errmsg("support function number %d is invalid for access method %s",
346+
op->number, "gist")));
347+
break;
348+
}
349+
}
350+
}

‎src/backend/access/hash/hash.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ hashhandler(PG_FUNCTION_ARGS)
8989
amroutine->amproperty = NULL;
9090
amroutine->ambuildphasename = NULL;
9191
amroutine->amvalidate = hashvalidate;
92+
amroutine->amadjustmembers = hashadjustmembers;
9293
amroutine->ambeginscan = hashbeginscan;
9394
amroutine->amrescan = hashrescan;
9495
amroutine->amgettuple = hashgettuple;

‎src/backend/access/hash/hashvalidate.c

Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
#include "access/amvalidate.h"
1717
#include "access/hash.h"
1818
#include "access/htup_details.h"
19+
#include "access/xact.h"
20+
#include "catalog/pg_am.h"
1921
#include "catalog/pg_amop.h"
2022
#include "catalog/pg_amproc.h"
2123
#include "catalog/pg_opclass.h"
@@ -25,6 +27,7 @@
2527
#include "parser/parse_coerce.h"
2628
#include "utils/builtins.h"
2729
#include "utils/fmgroids.h"
30+
#include "utils/lsyscache.h"
2831
#include "utils/regproc.h"
2932
#include "utils/syscache.h"
3033

@@ -341,3 +344,96 @@ check_hash_func_signature(Oid funcid, int16 amprocnum, Oid argtype)
341344
ReleaseSysCache(tp);
342345
return result;
343346
}
347+
348+
/*
349+
* Prechecking function for adding operators/functions to a hash opfamily.
350+
*/
351+
void
352+
hashadjustmembers(Oid opfamilyoid,
353+
Oid opclassoid,
354+
List *operators,
355+
List *functions)
356+
{
357+
Oid opcintype;
358+
ListCell *lc;
359+
360+
/*
361+
* Hash operators and required support functions are always "loose"
362+
* members of the opfamily if they are cross-type. If they are not
363+
* cross-type, we prefer to tie them to the appropriate opclass ... but if
364+
* the user hasn't created one, we can't do that, and must fall back to
365+
* using the opfamily dependency. (We mustn't force creation of an
366+
* opclass in such a case, as leaving an incomplete opclass laying about
367+
* would be bad. Throwing an error is another undesirable alternative.)
368+
*
369+
* This behavior results in a bit of a dump/reload hazard, in that the
370+
* order of restoring objects could affect what dependencies we end up
371+
* with. pg_dump's existing behavior will preserve the dependency choices
372+
* in most cases, but not if a cross-type operator has been bound tightly
373+
* into an opclass. That's a mistake anyway, so silently "fixing" it
374+
* isn't awful.
375+
*
376+
* Optional support functions are always "loose" family members.
377+
*
378+
* To avoid repeated lookups, we remember the most recently used opclass's
379+
* input type.
380+
*/
381+
if (OidIsValid(opclassoid))
382+
{
383+
/* During CREATE OPERATOR CLASS, need CCI to see the pg_opclass row */
384+
CommandCounterIncrement();
385+
opcintype = get_opclass_input_type(opclassoid);
386+
}
387+
else
388+
opcintype = InvalidOid;
389+
390+
/*
391+
* We handle operators and support functions almost identically, so rather
392+
* than duplicate this code block, just join the lists.
393+
*/
394+
foreach(lc, list_concat_copy(operators, functions))
395+
{
396+
OpFamilyMember *op = (OpFamilyMember *) lfirst(lc);
397+
398+
if (op->is_func && op->number != HASHSTANDARD_PROC)
399+
{
400+
/* Optional support proc, so always a soft family dependency */
401+
op->ref_is_hard = false;
402+
op->ref_is_family = true;
403+
op->refobjid = opfamilyoid;
404+
}
405+
else if (op->lefttype != op->righttype)
406+
{
407+
/* Cross-type, so always a soft family dependency */
408+
op->ref_is_hard = false;
409+
op->ref_is_family = true;
410+
op->refobjid = opfamilyoid;
411+
}
412+
else
413+
{
414+
/* Not cross-type; is there a suitable opclass? */
415+
if (op->lefttype != opcintype)
416+
{
417+
/* Avoid repeating this expensive lookup, even if it fails */
418+
opcintype = op->lefttype;
419+
opclassoid = opclass_for_family_datatype(HASH_AM_OID,
420+
opfamilyoid,
421+
opcintype);
422+
}
423+
if (OidIsValid(opclassoid))
424+
{
425+
/* Hard dependency on opclass */
426+
op->ref_is_hard = true;
427+
op->ref_is_family = false;
428+
op->refobjid = opclassoid;
429+
}
430+
else
431+
{
432+
/* We're stuck, so make a soft dependency on the opfamily */
433+
op->ref_is_hard = false;
434+
op->ref_is_family = true;
435+
op->refobjid = opfamilyoid;
436+
}
437+
}
438+
}
439+
}

0 commit comments

Comments
 (0)