Re: [HACKERS] WIP patch to improve amvalidate functions

[Alvaro Herrera]

Tom Lane wrote:

> I'm posting this now just in case anyone has some comments, or quibbles
> about the overall intent.  In particular, if anyone has an idea for a more
> thorough missing-objects check on BRIN opfamilies, I would like to hear
> it.  The fact that there are two kinds of opfamilies with rather different
> internal consistency requirements is a real pain there, and the check
> rules I have here are definitely the result of some trial and error.

Without reading your code: maybe each opfamily framework could itself
provide a validator function as a separate support procedure (currently
brin_minmax_validate and brin_inclusion_validate); so the generic BRIN
amvalidate verifies the basic elements of the opfamily, then hands off
to the opfamily-framework-specific validator function for additional
checking.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] WIP patch to improve amvalidate functions

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> I'm posting this now just in case anyone has some comments, or quibbles
>> about the overall intent.  In particular, if anyone has an idea for a more
>> thorough missing-objects check on BRIN opfamilies, I would like to hear
>> it.  The fact that there are two kinds of opfamilies with rather different
>> internal consistency requirements is a real pain there, and the check
>> rules I have here are definitely the result of some trial and error.

> Without reading your code: maybe each opfamily framework could itself
> provide a validator function as a separate support procedure (currently
> brin_minmax_validate and brin_inclusion_validate); so the generic BRIN
> amvalidate verifies the basic elements of the opfamily, then hands off
> to the opfamily-framework-specific validator function for additional
> checking.

Yeah, there were already some discussions in the index-AM-API-rewrite
thread about providing ways for individual opclasses to help in checking.
BRIN's not the only AM with an issue --- GIST, GIN, SPGIST all have the
property that only an individual opclass really knows which operator
strategy numbers are valid for it.  OTOH, I'm not certain how important
that is for those AMs, because their opclasses are fairly self-contained.
BRIN seems to have a two-level structure here which makes it more
important to offer cross-checking.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] WIP patch to improve amvalidate functions

[Tom Lane]

I spent some time trying to improve the amvalidate functions that were
committed in a rather crude state in 65c5fcd353a859da.  Attached is a
very-much-WIP patch showing where I'm headed:

* Check operator and function signatures (argument and result types)
not only their strategy numbers.

* Apply more thorough checks for missing operators and support functions.

* Don't throw ERROR if avoidable, because doing it like that means you can
only identify one problem per run.  Instead, print messages at INFO level
and make the amvalidate function return false if there are any complaints.
(A credible alternative would be to print the messages as WARNINGs, but
I think INFO is actually the right choice here, since the point of calling
amvalidate is to see those messages.)

* Make the messages more user-friendly by printing object names not just
OIDs.

I went through the amvalidate modules in the order they're shown in the
attached patch, and only the last (brinvalidate) is really fully
developed.  I need to go back and bring the others up to snuff, including
getting down to just one copy of the support routines that I wrote.
(I'm thinking of dumping those into a new file access/index/amvalidate.c.)
Also, I think the identify_opfamily_groups() function I wrote for BRIN
will allow improvement of the other modules' cross-checks for missing
operators/functions, but I've not tried yet.

Also, after this is done some of the queries in opr_sanity.sql will be
redundant and removable, but I didn't do that here.

I'm posting this now just in case anyone has some comments, or quibbles
about the overall intent.  In particular, if anyone has an idea for a more
thorough missing-objects check on BRIN opfamilies, I would like to hear
it.  The fact that there are two kinds of opfamilies with rather different
internal consistency requirements is a real pain there, and the check
rules I have here are definitely the result of some trial and error.

Comments?

regards, tom lane

diff --git a/src/backend/access/nbtree/nbtvalidate.c b/src/backend/access/nbtree/nbtvalidate.c
index b814b54..2c03688 100644
*** a/src/backend/access/nbtree/nbtvalidate.c
--- b/src/backend/access/nbtree/nbtvalidate.c
***
*** 18,28 
--- 18,37 
  #include "catalog/pg_amop.h"
  #include "catalog/pg_amproc.h"
  #include "catalog/pg_opclass.h"
+ #include "catalog/pg_operator.h"
+ #include "catalog/pg_proc.h"
+ #include "catalog/pg_type.h"
  #include "utils/builtins.h"
  #include "utils/catcache.h"
  #include "utils/syscache.h"
  
  
+ static bool check_func_signature(Oid funcid, Oid restype,
+ 	 int nargs, Oid arg1type, Oid arg2type);
+ static bool check_opr_signature(Oid opno, Oid restype,
+ 	Oid lefttype, Oid righttype);
+ 
+ 
  /*
   * Validator for a btree opclass.
   *
*** btvalidate(Oid opclassoid)
*** 77,90 
  		HeapTuple	proctup = >members[i]->tuple;
  		Form_pg_amproc procform = (Form_pg_amproc) GETSTRUCT(proctup);
  
! 		/* Check that only allowed procedure numbers exist */
! 		if (procform->amprocnum != BTORDER_PROC &&
! 			procform->amprocnum != BTSORTSUPPORT_PROC)
! 			ereport(ERROR,
! 	(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
! 	 errmsg("btree opfamily %u contains invalid support number %d for procedure %u",
! 			opfamilyoid,
! 			procform->amprocnum, procform->amproc)));
  
  		/* Remember functions that are specifically for the named opclass */
  		if (procform->amproclefttype == opcintype &&
--- 86,121 
  		HeapTuple	proctup = >members[i]->tuple;
  		Form_pg_amproc procform = (Form_pg_amproc) GETSTRUCT(proctup);
  
! 		/* Check procedure numbers and function signatures */
! 		switch (procform->amprocnum)
! 		{
! 			case BTORDER_PROC:
! /* comparison func must match the opclass entry */
! if (!check_func_signature(procform->amproc, INT4OID, 2,
! 		  procform->amproclefttype,
! 		  procform->amprocrighttype))
! 	ereport(ERROR,
! 			(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
! 			 errmsg("btree opfamily %u contains support procedure %u with wrong signature",
! 	opfamilyoid, procform->amproc)));
! break;
! 			case BTSORTSUPPORT_PROC:
! /* sortsupport always takes internal and returns void */
! if (!check_func_signature(procform->amproc, VOIDOID, 1,
! 		  INTERNALOID, InvalidOid))
! 	ereport(ERROR,
! 			(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
! 			 errmsg("btree opfamily %u contains support procedure %u with wrong signature",
! 	opfamilyoid, procform->amproc)));
! break;
! 			default:
! ereport(ERROR,
! 		(errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
! 		 errmsg("btree opfamily %u contains invalid support number %d for procedure %u",
! opfamilyoid,
! procform->amprocnum, procform->amproc)));
! break;
! 		}
  
  		/* Remember functions that are specifically for the named opclass */
  		if (procform->amproclefttype == opcintype &&