Re: [HACKERS] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
On Fri, Mar 25, 2016 at 12:39 PM, Tom Lane wrote: > I wrote: >> Robert Haas writes: >>> On Wed, Mar 23, 2016 at 12:27 PM, Tom Lane wrote: We could resolve both of these issues by changing the semantics of OprUpdate so that it unconditionally does a CommandCounterIncrement after each update that it performs. IMO that would be a lot simpler and more bulletproof; it'd allow removal of a lot of these overly-tightly-reasoned cases. > >>> I tried this, but it did not seem to work. > >> Odd. If you post the revised patch, I'll try to chase down what's wrong. > > After playing with this, I'll bet you forgot that RemoveOperatorById would > need to re-fetch the target tuple if it got updated. We could > alternatively fix that by skipping updates on the tuple due to be deleted, > but that would convolute the logic in OperatorUpd, which didn't seem > worthwhile to me. > > I found some other stuff needing fixing (mostly typos in comments) and > also realized that we don't really need to bother with heap_modify_tuple > at all. I pushed it with those fixes. Cool. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
I wrote: > Robert Haas writes: >> On Wed, Mar 23, 2016 at 12:27 PM, Tom Lane wrote: >>> We could resolve both of these issues by changing the semantics of >>> OprUpdate so that it unconditionally does a CommandCounterIncrement >>> after each update that it performs. IMO that would be a lot simpler >>> and more bulletproof; it'd allow removal of a lot of these >>> overly-tightly-reasoned cases. >> I tried this, but it did not seem to work. > Odd. If you post the revised patch, I'll try to chase down what's wrong. After playing with this, I'll bet you forgot that RemoveOperatorById would need to re-fetch the target tuple if it got updated. We could alternatively fix that by skipping updates on the tuple due to be deleted, but that would convolute the logic in OperatorUpd, which didn't seem worthwhile to me. I found some other stuff needing fixing (mostly typos in comments) and also realized that we don't really need to bother with heap_modify_tuple at all. I pushed it with those fixes. 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
Re: [HACKERS] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
Robert Haas writes: > On Wed, Mar 23, 2016 at 12:27 PM, Tom Lane wrote: >> I'm also a bit dubious of the assumption in RemoveOperatorById that an >> operator can't be its own negator. Yeah, that should not be the case, >> but if it is the case the deletion will fail outright. > So what? We've never guaranteed that things are going to work if you > start by corrupting the catalogs, and I wouldn't pick this as a place > to start. I would not be worried except that it breaks a case that used to work, as your test below demonstrates. >> We could resolve both of these issues by changing the semantics of >> OprUpdate so that it unconditionally does a CommandCounterIncrement >> after each update that it performs. IMO that would be a lot simpler >> and more bulletproof; it'd allow removal of a lot of these >> overly-tightly-reasoned cases. > I tried this, but it did not seem to work. Odd. If you post the revised patch, I'll try to chase down what's wrong. 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
Re: [HACKERS] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
On Wed, Mar 23, 2016 at 12:27 PM, Tom Lane wrote: > Robert Haas writes: >> I did not find this version very clear. It wasn't consistent about >> using ObjectIdGetDatum() where needed, but the bigger problem was that >> I found the logic unnecessarily convoluted. I rewrote it - I believe >> more straightforwardly - as attached. How does this look? > > I'd suggest that we save some code by always doing separate updates for > the commutator and negator entries. We can handle the corner case where > they're the same by doing a CommandCounterIncrement between the updates, > instead of having convoluted and probably-never-yet-tested logic. Sure, we could do that, but it isn't necessary. If the logic never gets hit, the question of whether it has bugs isn't that important. And I'd rather not rejigger things more than necessary in something that's going to be back-patched. > I'm also a bit dubious of the assumption in RemoveOperatorById that an > operator can't be its own negator. Yeah, that should not be the case, > but if it is the case the deletion will fail outright. So what? We've never guaranteed that things are going to work if you start by corrupting the catalogs, and I wouldn't pick this as a place to start. > We could resolve both of these issues by changing the semantics of > OprUpdate so that it unconditionally does a CommandCounterIncrement > after each update that it performs. IMO that would be a lot simpler > and more bulletproof; it'd allow removal of a lot of these > overly-tightly-reasoned cases. I tried this, but it did not seem to work. With the command counter increments added and the conditional logic removed, I got: rhaas=# CREATE OPERATOR === (PROCEDURE = int8eq, LEFTARG = bigint, RIGHTARG = bigint); CREATE OPERATOR rhaas=# update pg_operator set oprnegate = oid where oprname = '==='; UPDATE 1 rhaas=# drop operator === (bigint, bigint); ERROR: attempted to delete invisible tuple The same test case without those changes fails with: ERROR: tuple already updated by self Interestingly, that test case passes on unpatched master. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
Robert Haas writes: > I did not find this version very clear. It wasn't consistent about > using ObjectIdGetDatum() where needed, but the bigger problem was that > I found the logic unnecessarily convoluted. I rewrote it - I believe > more straightforwardly - as attached. How does this look? I'd suggest that we save some code by always doing separate updates for the commutator and negator entries. We can handle the corner case where they're the same by doing a CommandCounterIncrement between the updates, instead of having convoluted and probably-never-yet-tested logic. I'm also a bit dubious of the assumption in RemoveOperatorById that an operator can't be its own negator. Yeah, that should not be the case, but if it is the case the deletion will fail outright. We could resolve both of these issues by changing the semantics of OprUpdate so that it unconditionally does a CommandCounterIncrement after each update that it performs. IMO that would be a lot simpler and more bulletproof; it'd allow removal of a lot of these overly-tightly-reasoned cases. 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
Re: [HACKERS] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
On Tue, Mar 22, 2016 at 9:25 PM, Tomas Vondra
wrote:
> OK, the new code seems more comprehensible to me.
I did not find this version very clear. It wasn't consistent about
using ObjectIdGetDatum() where needed, but the bigger problem was that
I found the logic unnecessarily convoluted. I rewrote it - I believe
more straightforwardly - as attached. How does this look?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
diff --git a/src/backend/catalog/pg_operator.c b/src/backend/catalog/pg_operator.c
index 3cd1899..cd66823 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -54,8 +54,6 @@ static Oid OperatorShellMake(const char *operatorName,
Oid leftTypeId,
Oid rightTypeId);
-static void OperatorUpd(Oid baseId, Oid commId, Oid negId);
-
static Oid get_other_operator(List *otherOp,
Oid otherLeftTypeId, Oid otherRightTypeId,
const char *operatorName, Oid operatorNamespace,
@@ -566,7 +564,7 @@ OperatorCreate(const char *operatorName,
commutatorId = operatorObjectId;
if (OidIsValid(commutatorId) || OidIsValid(negatorId))
- OperatorUpd(operatorObjectId, commutatorId, negatorId);
+ OperatorUpd(operatorObjectId, commutatorId, negatorId, false);
return address;
}
@@ -639,125 +637,158 @@ get_other_operator(List *otherOp, Oid otherLeftTypeId, Oid otherRightTypeId,
* OperatorUpd
*
* For a given operator, look up its negator and commutator operators.
- * If they are defined, but their negator and commutator fields
- * (respectively) are empty, then use the new operator for neg or comm.
- * This solves a problem for users who need to insert two new operators
- * which are the negator or commutator of each other.
+ * When isDelete is false, update their negator and commutator operators to
+ * point back to the given operator; when isDelete is true, update those
+ * operators to no longer point back to the given operator.
+ *
+ * The !isDelete case solves a problem for users who need to insert two new
+ * operators which are the negator or commutator of each other, while the
+ * isDelete case is important so as not to leave dangling OID links behind
+ * after dropping an operator.
*/
-static void
-OperatorUpd(Oid baseId, Oid commId, Oid negId)
+void
+OperatorUpd(Oid baseId, Oid commId, Oid negId, bool isDelete)
{
- int i;
Relation pg_operator_desc;
- HeapTuple tup;
- bool nulls[Natts_pg_operator];
- bool replaces[Natts_pg_operator];
- Datum values[Natts_pg_operator];
-
- for (i = 0; i < Natts_pg_operator; ++i)
- {
- values[i] = (Datum) 0;
- replaces[i] = false;
- nulls[i] = false;
- }
+ HeapTuple tup = NULL;
/*
- * check and update the commutator & negator, if necessary
- *
- * We need a CommandCounterIncrement here in case of a self-commutator
- * operator: we'll need to update the tuple that we just inserted.
+ * If we're making an operator into its own commutator, then we need a
+ * command-counter increment here, since we've just inserted the tuple
+ * we're about to update. But when we're dropping an operator, we can
+ * skip this.
*/
- CommandCounterIncrement();
+ if (!isDelete)
+ CommandCounterIncrement();
+ /* Open the relation. */
pg_operator_desc = heap_open(OperatorRelationId, RowExclusiveLock);
- tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(commId));
+ /* Get a copy of the commutator's tuple. */
+ if (OidIsValid(commId))
+ tup = SearchSysCacheCopy1(OPEROID, ObjectIdGetDatum(commId));
- /*
- * if the commutator and negator are the same operator, do one update. XXX
- * this is probably useless code --- I doubt it ever makes sense for
- * commutator and negator to be the same thing...
- */
- if (commId == negId)
+ /* Update the commutator's tuple if need be. */
+ if (HeapTupleIsValid(tup))
{
- if (HeapTupleIsValid(tup))
+ Form_pg_operator t = (Form_pg_operator) GETSTRUCT(tup);
+ bool update_commutator = false;
+ bool nulls[Natts_pg_operator];
+ bool replaces[Natts_pg_operator];
+ Datum values[Natts_pg_operator];
+
+ memset(values, 0, sizeof(values));
+ memset(replaces, 0, sizeof(replaces));
+ memset(nulls, 0, sizeof(nulls));
+
+ /*
+ * Out of due caution, we only change the commutator's orpcom field
+ * if it has the exact value we expected: InvalidOid when creating an
+ * operator, and baseId when dropping one.
+ */
+ if (isDelete && t->oprcom == baseId)
{
- Form_pg_operator t = (Form_pg_operator) GETSTRUCT(tup);
+ values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(InvalidOid);
+ replaces[Anum_pg_operator_oprcom - 1] = true;
+ update_commutator = true;
+ }
+ else if (!isDelete && !OidIsValid(t->oprcom))
+ {
+ values[Anum_pg_operator_oprcom - 1] = ObjectIdGetDatum(baseId);
+ replaces[Anum_pg_operator_oprcom - 1] = true;
+ update_commutator = true;
+ }
- if (!OidIsValid(t->oprcom) || !OidIsValid(t->oprnegate))
+ /*
+ * If the commutator is also the negator, which doesn't
Re: [HACKERS] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
Hi, On 03/23/2016 12:50 AM, Roma Sokolov wrote: Hi, Tomas, thanks for review and comments! 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? Indeed, code in OperatorUpd is not very easy to read, due to handling this case. However we can achieve the same results without too much duplication. I have changed OperatorUpd to perform tuple modification in "lazy" way. Please, check it out in v4.patch (attached). OK, the new code seems more comprehensible to me. 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 We do not need to check for operOid != op->oprnegate, since we can't create operator that is negator to itself. Thus, opnergate either present and differs from operator being deleted, or is InvalidOid. I have added some clarification in the comment for future readers. Ah, I see. Thanks for the clarification. Fixed style issues as well. I've noticed some whitespace issues in the OperatorUpd function - there are two or three lines with just a tabulator at the beginning, and one comment mixes indentation by tabs with spaces. Also, it's generally recommended no to tweak formatting when not necessary, so perhaps don't remove the empty line at the end of the function (before simple_heap_update). I think the comments will need rewording, but I'll leave that to a native speaker. regards Tomas -- Tomas Vondra http://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] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
Hi, Tomas, thanks for review and comments! > 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? Indeed, code in OperatorUpd is not very easy to read, due to handling this case. However we can achieve the same results without too much duplication. I have changed OperatorUpd to perform tuple modification in "lazy" way. Please, check it out in v4.patch (attached). > 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 We do not need to check for operOid != op->oprnegate, since we can't create operator that is negator to itself. Thus, opnergate either present and differs from operator being deleted, or is InvalidOid. I have added some clarification in the comment for future readers. Fixed style issues as well. Cheers, Roma fix_drop_operator_reset_oprcom_and_oprnegate_fields_v4.patch Description: Binary data -- 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] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
Hi, On 03/01/2016 05:08 PM, Roma Sokolov wrote: On 27 Feb 2016, at 03:46, Euler Taveira 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 of a 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 not self-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 -- 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] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
> On 27 Feb 2016, at 03:46, Euler Taveira 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. Cheers, Roma fix_drop_operator_reset_oprcom_and_oprnegate_fields_v3.patch Description: Binary data -- 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] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
Roma Sokolov wrote: See v2 of the patch attached. Hello. I have a stylistic comments. Sometimes you forget a space: + replaces[Anum_pg_operator_oprcom - 1] =true; or use tab insted space: + if (OidIsValid(op->oprnegate) || + (OidIsValid(op->oprcom) && operOid != op->oprcom)) + OperatorUpd(operOid, + operOid == op->oprcom ? InvalidOid : op->oprcom, + op->oprnegate, + true); And I think if you make this logic into a separate function, it is possible to simplify the code. OperatorUpd function is too complex. Also better to add comments to the tests. The rest seems good. PS I here thought it would be possible to print operators that have been changed? -- Yury Zhuravlev Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- 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] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
On Sat, Feb 27, 2016 at 12:46 AM, Roma Sokolov wrote: > Should this patch be added to CommitFest? Yes please. -- Michael -- 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] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
On 26-02-2016 17:51, Roma Sokolov wrote: > Fixed the patch to be more descriptive and to avoid repeating same > computation over and over again. See v2 of the patch attached. > Oh, much better. > Why do you think that? Should I remove them or maybe send as separate > patch? > 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. If you can reduce it to a small set of commands using some of the oidjoins.sql queries, I think it could be sufficient. -- Euler Taveira Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento -- 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] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
Thanks for comments, Euler! > ... is hard to understand. Instead, you could separate the conditional > expression into a variable. Fixed the patch to be more descriptive and to avoid repeating same computation over and over again. See v2 of the patch attached. > I don't think those are mandatory. Why do you think that? Should I remove them or maybe send as separate patch? Cheers, Roma fix_drop_operator_reset_oprcom_and_oprnegate_fields_v2.patch Description: Binary data -- 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] [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator
On 26-02-2016 12:46, Roma Sokolov wrote: > Regression tests are added to check DROP OPERATOR behaves as intended > (including > case with self-commutator and unlikely case with operator being both negator > and > commutator). > I don't think those are mandatory. > Should this patch be added to CommitFest? > Why not? I didn't test your patch but + if (isDelete ? (t->oprcom == baseId || t->oprnegate == baseId) + : !OidIsValid(t->oprcom) || !OidIsValid(t->oprnegate)) ... is hard to understand. Instead, you could separate the conditional expression into a variable. + if (isDelete ? t->oprnegate == baseId : !OidIsValid(t->oprnegate)) It could be separate into a variable to be readable (or at least deserve a comment). (isDelete ? InvalidOid : ObjectIdGetDatum(baseId)) ... and this one too. It is used in 4 places in that function. -- Euler Taveira Timbira - http://www.timbira.com.br/ PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
