Re: [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator - Mailing list pgsql-hackers

Hi,

On 03/01/2016 05:08 PM, Roma Sokolov wrote:
>> On 27 Feb 2016, at 03:46, Euler Taveira <euler@timbira.com.br> wrote:
>> Because it is not a common practice to test catalog dependency on
>> separate tests (AFAICS initial catalogs are tested with oidjoins.sql).
>> Also, your test case is too huge for such a small use case.
>
> It seems oidjoins.sql is automatically generated and contains tests only for
> initial database state. On the other hand, there are tests for CREATE OPERATOR
> and ALTER OPERATOR, so it seems reasonable to me to have separate DROP OPERATOR
> test, or to move all operator related testing to one file. This is however
> clearly outside of the scope of this patch, so in v3 I've simplified tests using
> queries from oidjoins.sql.

I think it's OK to have a separate regression test for this. Clearly 
oidjoins is not a good place for such tests, and as you point out there 
are already tests for CREATE/ALTER OPERATOR, so it's perfectly natural 
to add a new file for DROP OPERATOR. I don't think it contradicts any 
common practice, as the existing CREATE/ALTER OPERATOR tests do pretty 
much the same thing.

One comment for the tests, though - you're using a mix of tabs and 
spaces for indentation, which breaks psql unpredictably (when debugging 
and pasting the commands to psql). Which is a bit annoying.

A few more comments:

1) OperatorUpd (pg_operator.c)
------------------------------
  /*   * check and update the commutator & negator, if necessary   *   * We need a CommandCounterIncrement here in case
ofa self-commutator   * operator: we'll need to update the tuple that we just inserted.   */  if (!isDelete)
CommandCounterIncrement();

This would really deserve an explanation of why we don't need to 
increment the command counter for a delete.
  /* When deleting, reset other operator field to InvalidOid, otherwise,   * set it to point to operator being updated
*/
 

This comment is a bit broken - the first line should be just '/*' and 
the second line uses spaces instead of a tabulator.

I have to admit I find the existing code a bit convoluted, particularly 
the part that deals with the (commId == negId) case. And the patch does 
not really improve the situation, quite the contrary.

Perhaps it's time to get rid of this optimization? I mean, how likely it 
is to have an operator with the same negator and commutator? And how 
often we do DROP OPERATOR? Apparently even the original author doubted 
this, according to the comment right in front of the block.

2) operatorcmds.c
------------------

/* * Reset links on commutator and negator. Only do that if either * oprcom or oprnegate is set and given operator is
notself-commutator. * For self-commutator with negator prevent meaningful updates of the * same tuple by sending
InvalidOid.*/
 
if (OidIsValid(op->oprnegate) ||    (OidIsValid(op->oprcom) && operOid != op->oprcom))    OperatorUpd(operOid,
 operOid == op->oprcom ? InvalidOid : op->oprcom,            op->oprnegate,            true);
 

Firstly, this block contains tabulators within both the comment and the 
code (e.g. "is not" or in front of the "&&" operator. That seems a bit 
broken, I guess.

Also, maybe I'm missing something obvious, but it's not immediately 
obvious to me why we're only checking oprcom and not oprnegate? I.e. why 
shouldn't the code be

if (OidIsValid(op->oprnegate) ||    (OidIsValid(op->oprcom) && operOid != op->oprcom) ||    (OidIsValid(op->oprnegate)
&&operOid != op->oprnegate))    OperatorUpd(operOid,            operOid == op->oprcom ? InvalidOid : op->oprcom,
   operOid == op->oprnegate ? InvalidOid : op->oprnegate,            true);
 


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services