Обсуждение: BUG #17151: A SEGV in optimizer

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 ) ;
 ;
Asan Report:
AddressSanitizer:DEADLYSIGNAL
=================================================================
E ( v1==82==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000032
(pc 0x7f910c8c936f bp 0x7ffecf4f37f0 sp 0x7ffecf4f2f78 T0)
 ) ;
 ;==82==The signal is caused by a READ memory access.
==82==Hint: address points to the zero page.
    #0 0x7f910c8c936e
/build/glibc-S9d2JN/glibc-2.27/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:311
    #1 0x5581ad in __asan_memcpy (/usr/local/pgsql/bin/postgres+0x5581ad)
    #2 0xd45b8f in ExecLockRows
/home/postgres/postgres/bld/../src/backend/executor/nodeLockRows.c:161:9
    #3 0xd42768 in ExecProcNode
/home/postgres/postgres/bld/../src/include/executor/executor.h:257:9
    #4 0xd42768 in ExecLimit
/home/postgres/postgres/bld/../src/backend/executor/nodeLimit.c:96
    #5 0xcb1514 in ExecScanFetch
/home/postgres/postgres/bld/../src/backend/executor/execScan.c:133:9
    #6 0xcb0849 in ExecScan
/home/postgres/postgres/bld/../src/backend/executor/execScan.c:199:10
    #7 0xd58112 in ExecProcNode
/home/postgres/postgres/bld/../src/include/executor/executor.h:257:9
    #8 0xd58112 in ExecModifyTable
/home/postgres/postgres/bld/../src/backend/executor/nodeModifyTable.c:2423
    #9 0xc8a56c in ExecProcNode
/home/postgres/postgres/bld/../src/include/executor/executor.h:257:9
    #10 0xc8a56c in ExecutePlan
/home/postgres/postgres/bld/../src/backend/executor/execMain.c:1551
    #11 0xc8a56c in standard_ExecutorRun
/home/postgres/postgres/bld/../src/backend/executor/execMain.c:361
    #12 0xc89061 in ExecutorRun
/home/postgres/postgres/bld/../src/backend/executor/execMain.c:305:3
    #13 0x13ce4d6 in ProcessQuery
/home/postgres/postgres/bld/../src/backend/tcop/pquery.c:160:2
    #14 0x13cb899 in PortalRunMulti
/home/postgres/postgres/bld/../src/backend/tcop/pquery.c
    #15 0x13c9708 in PortalRun
/home/postgres/postgres/bld/../src/backend/tcop/pquery.c:786:5
    #16 0x13c52d5 in exec_simple_query
/home/postgres/postgres/bld/../src/backend/tcop/postgres.c:1214:10
    #17 0x13be613 in PostgresMain
/home/postgres/postgres/bld/../src/backend/tcop/postgres.c
    #18 0xe073fd in main
/home/postgres/postgres/bld/../src/backend/main/main.c:205:3
    #19 0x7f910c82fbf6 in __libc_start_main
/build/glibc-S9d2JN/glibc-2.27/csu/../csu/libc-start.c:310
    #20 0x499889 in _start (/usr/local/pgsql/bin/postgres+0x499889)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV
/build/glibc-S9d2JN/glibc-2.27/string/../sysdeps/x86_64/multiarch/memmove-vec-unaligned-erms.S:311
==82==ABORTING

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

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 438b077004..f715748d88 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -3177,6 +3177,8 @@ transformLockingClause(ParseState *pstate, Query *qry, LockingClause *lc,
             RangeTblEntry *rte = (RangeTblEntry *) lfirst(rt);

             ++i;
+            if (!rte->inFromCl)
+                continue;        /* ignore OLD/NEW in rules */
             switch (rte->rtekind)
             {
                 case RTE_RELATION:
@@ -3227,6 +3229,8 @@ transformLockingClause(ParseState *pstate, Query *qry, LockingClause *lc,
                 RangeTblEntry *rte = (RangeTblEntry *) lfirst(rt);

                 ++i;
+                if (!rte->inFromCl)
+                    continue;    /* ignore OLD/NEW in rules */
                 if (strcmp(rte->eref->aliasname, thisrel->relname) == 0)
                 {
                     switch (rte->rtekind)

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

diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 438b077004..15669c8238 100644
--- a/src/backend/parser/analyze.c
+++ b/src/backend/parser/analyze.c
@@ -3170,13 +3170,22 @@ transformLockingClause(ParseState *pstate, Query *qry, LockingClause *lc,

     if (lockedRels == NIL)
     {
-        /* all regular tables used in query */
+        /*
+         * Lock all regular tables used in query and its subqueries.  We
+         * examine inFromCl to exclude auto-added RTEs, particularly NEW/OLD
+         * in rules.  This is a bit of an abuse of a mostly-obsolete flag, but
+         * it's convenient.  We can't rely on the namespace mechanism that has
+         * largely replaced inFromCl, since for example we need to lock
+         * base-relation RTEs even if they are masked by upper joins.
+         */
         i = 0;
         foreach(rt, qry->rtable)
         {
             RangeTblEntry *rte = (RangeTblEntry *) lfirst(rt);

             ++i;
+            if (!rte->inFromCl)
+                continue;
             switch (rte->rtekind)
             {
                 case RTE_RELATION:
@@ -3206,7 +3215,11 @@ transformLockingClause(ParseState *pstate, Query *qry, LockingClause *lc,
     }
     else
     {
-        /* just the named tables */
+        /*
+         * Lock just the named tables.  As above, we allow locking any base
+         * relation regardless of alias-visibility rules, so we need to
+         * examine inFromCl to exclude OLD/NEW.
+         */
         foreach(l, lockedRels)
         {
             RangeVar   *thisrel = (RangeVar *) lfirst(l);
@@ -3227,6 +3240,8 @@ transformLockingClause(ParseState *pstate, Query *qry, LockingClause *lc,
                 RangeTblEntry *rte = (RangeTblEntry *) lfirst(rt);

                 ++i;
+                if (!rte->inFromCl)
+                    continue;
                 if (strcmp(rte->eref->aliasname, thisrel->relname) == 0)
                 {
                     switch (rte->rtekind)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index e28248af32..7af13dee43 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -929,10 +929,10 @@ typedef struct PartitionCmd
  *      inFromCl marks those range variables that are listed in the FROM clause.
  *      It's false for RTEs that are added to a query behind the scenes, such
  *      as the NEW and OLD variables for a rule, or the subqueries of a UNION.
- *      This flag is not used anymore during parsing, since the parser now uses
- *      a separate "namespace" data structure to control visibility, but it is
- *      needed by ruleutils.c to determine whether RTEs should be shown in
- *      decompiled queries.
+ *      This flag is not used during parsing (except in transformLockingClause,
+ *      q.v.); the parser now uses a separate "namespace" data structure to
+ *      control visibility.  But it is needed by ruleutils.c to determine
+ *      whether RTEs should be shown in decompiled queries.
  *
  *      requiredPerms and checkAsUser specify run-time access permissions
  *      checks to be performed at query startup.  The user must have *all*

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