Bug in MERGE's test for tables with rules

Started by Dean Rasheedover 3 years ago3 messageshackers
Jump to latest
#1Dean Rasheed
dean.a.rasheed@gmail.com

While playing around with rules and MERGE, I noticed that there is a
bug in the way that it detects whether the target table has rules ---
it uses rd_rel->relhasrules, which can be incorrect, since it might be
set for a table that doesn't currently have rules, but did in the
recent past.

So it actually needs to examine rd_rules. Technically, I think that it
would be sufficient to just test whether rd_rules is non-NULL, but I
think it's more robust and readable to check rd_rules->numLocks, as in
the attached patch.

Regards,
Dean

Attachments:

fix-merge-rule-detection.patchtext/x-patch; charset=US-ASCII; name=fix-merge-rule-detection.patchDownload+41-1
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Dean Rasheed (#1)
Re: Bug in MERGE's test for tables with rules

Dean Rasheed <dean.a.rasheed@gmail.com> writes:

While playing around with rules and MERGE, I noticed that there is a
bug in the way that it detects whether the target table has rules ---
it uses rd_rel->relhasrules, which can be incorrect, since it might be
set for a table that doesn't currently have rules, but did in the
recent past.

So it actually needs to examine rd_rules. Technically, I think that it
would be sufficient to just test whether rd_rules is non-NULL, but I
think it's more robust and readable to check rd_rules->numLocks, as in
the attached patch.

+1 for the code change. Not quite sure the added test case is worth
the cycles.

regards, tom lane

#3Dean Rasheed
dean.a.rasheed@gmail.com
In reply to: Tom Lane (#2)
Re: Bug in MERGE's test for tables with rules

On Wed, 23 Nov 2022 at 15:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Not quite sure the added test case is worth the cycles.

No, probably not, for such a trivial change.

Pushed to HEAD and 15, without the test. Thanks for looking!

Regards,
Dean