BUG #17151: A SEGV in optimizer

On Wed, Aug 18, 2021 at 02:53:07AM +0000, PG Bug reporting form wrote:

The following bug has been logged on the website:

Bug reference: 17151
Logged by: Zhiyong Wu
Email address: 253540651@qq.com
PostgreSQL version: 14beta2
Operating system: Linux version 5.13.0-1-MANJARO (builduser@LEGION)
Description:

PoC:
CREATE TEMPORARY TABLE v0 ( v1 INTEGER ) ;
CREATE RULE v1 AS ON DELETE TO v0 DO INSERT INTO v0 SELECT OFFSET - - - -1
FOR UPDATE FOR UPDATE FOR SHARE FOR UPDATE FOR SHARE FOR SHARE ;
;
DELETE FROM v0 ;
ALTER TABLE v0 ADD UNIQUE ( v1 ) ;
;

I can confirm this failure on git master.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

If only the physical world exists, free will is an illusion.

On Wed, Aug 18, 2021 at 4:12 PM PG Bug reporting form
<noreply@postgresql.org> wrote:

The following bug has been logged on the website:

Bug reference: 17151
Logged by: Zhiyong Wu
Email address: 253540651@qq.com
PostgreSQL version: 14beta2
Operating system: Linux version 5.13.0-1-MANJARO (builduser@LEGION)
Description:

PoC:
CREATE TEMPORARY TABLE v0 ( v1 INTEGER ) ;
CREATE RULE v1 AS ON DELETE TO v0 DO INSERT INTO v0 SELECT OFFSET - - - -1
FOR UPDATE FOR UPDATE FOR SHARE FOR UPDATE FOR SHARE FOR SHARE ;
;
DELETE FROM v0 ;
ALTER TABLE v0 ADD UNIQUE ( v1 ) ;

Thank you for reporting!

This seems to happen on all supported versions. A segmentation fault
happens during "DELETE FROM v0". The minimum reproduce steps are:

CREATE TABLE v0 ( v1 INTEGER ) ;
CREATE RULE v1 AS
ON DELETE TO v0 DO SELECT
FOR UPDATE OF old;
DELETE FROM v0 ;

The plan of the DELETE statement is:

QUERY PLAN
-------------------------------------------------------------------
LockRows (cost=0.00..0.02 rows=1 width=6)
Output: v0.ctid
-> Result (cost=0.00..0.01 rows=1 width=6)
Output: v0.ctid

Delete on public.v0 (cost=0.00..35.50 rows=0 width=0)
-> Seq Scan on public.v0 (cost=0.00..35.50 rows=2550 width=6)
Output: ctid
(8 rows)

What happens in the DELETE statement is that LockRows node receives a
tuple from Result node and attempts to acquire a row lock on it but it
shouldn't. I guess that analyzing the SELECT query in rule v1 should
raise an error in the first place since there is no 'old' table in its
FROM clause. I'm still investigating this issue.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

Masahiko Sawada <sawada.mshk@gmail.com> writes:

This seems to happen on all supported versions. A segmentation fault
happens during "DELETE FROM v0". The minimum reproduce steps are:

CREATE TABLE v0 ( v1 INTEGER ) ;
CREATE RULE v1 AS
ON DELETE TO v0 DO SELECT
FOR UPDATE OF old;
DELETE FROM v0 ;

What happens in the DELETE statement is that LockRows node receives a
tuple from Result node and attempts to acquire a row lock on it but it
shouldn't. I guess that analyzing the SELECT query in rule v1 should
raise an error in the first place since there is no 'old' table in its
FROM clause. I'm still investigating this issue.

Agreed, this ought to be rejected. In the original case, OLD wasn't
mentioned explicitly, but the parser still attempted to lock it,
which is a slightly different bug.

I think that the fix might be as easy as the attached. In your
example, this would result in
ERROR: relation "old" in FOR UPDATE clause not found in FROM clause
which is not terribly specific, but it's not really wrong either.
Seeing that nobody has complained of this in ~20 years, I doubt
we need to work harder on the error message.

(This passes check-world, but I've not double-checked to make sure
that inFromCl will be set in exactly the cases we want.)

regards, tom lane

I wrote:

(This passes check-world, but I've not double-checked to make sure
that inFromCl will be set in exactly the cases we want.)

After studying the code a bit more, I remembered why my hindbrain
was feeling uncomfortable about that coding: parsenodes.h says that
inFromCl is quasi-deprecated and not used anymore during parsing.

However, we can't really use the ParseNamespaceItem data structure
for this purpose, because baserels should be available to lock
whether or not they are visible according to join aliasing rules.
I don't see a lot of point to inventing some complicated add-on
for this when inFromCl will serve fine. So I think we should just
adjust the relevant comments, say like the attached.

We probably need some regression test cases added (I wonder whether
FOR UPDATE in rule actions is covered at all ATM). Otherwise
I feel like this is OK to commit.

regards, tom lane

On Thu, Aug 19, 2021 at 8:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

(This passes check-world, but I've not double-checked to make sure
that inFromCl will be set in exactly the cases we want.)

After studying the code a bit more, I remembered why my hindbrain
was feeling uncomfortable about that coding: parsenodes.h says that
inFromCl is quasi-deprecated and not used anymore during parsing.

However, we can't really use the ParseNamespaceItem data structure
for this purpose, because baserels should be available to lock
whether or not they are visible according to join aliasing rules.
I don't see a lot of point to inventing some complicated add-on
for this when inFromCl will serve fine. So I think we should just
adjust the relevant comments, say like the attached.

The patch looks good to me. I've also confirmed that it passed
check-world and fixed the problem.

We probably need some regression test cases added (I wonder whether
FOR UPDATE in rule actions is covered at all ATM). Otherwise
I feel like this is OK to commit.

+1 for adding at least two queries reported on this thread to regression tests.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/

Masahiko Sawada <sawada.mshk@gmail.com> writes:

On Thu, Aug 19, 2021 at 8:24 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

We probably need some regression test cases added (I wonder whether
FOR UPDATE in rule actions is covered at all ATM). Otherwise
I feel like this is OK to commit.

+1 for adding at least two queries reported on this thread to regression tests.

Right, pushed with some test cases.

regards, tom lane