Horton

Thread

Commits

  1. Avoid trying to lock OLD/NEW in a rule with FOR UPDATE.

  1. BUG #17151: A SEGV in optimizer

    The Post Office <noreply@postgresql.org> — 2021-08-18T02:53:07Z

    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

  2. Re: BUG #17151: A SEGV in optimizer

    Bruce Momjian <bruce@momjian.us> — 2021-08-18T11:51:19Z

    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.

  3. Re: BUG #17151: A SEGV in optimizer

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-08-18T11:52:51Z

    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/

  4. Re: BUG #17151: A SEGV in optimizer

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-18T15:55:58Z

    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

  5. Re: BUG #17151: A SEGV in optimizer

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-18T23:24:20Z

    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

  6. Re: BUG #17151: A SEGV in optimizer

    Masahiko Sawada <sawada.mshk@gmail.com> — 2021-08-19T01:02:44Z

    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/

  7. Re: BUG #17151: A SEGV in optimizer

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-08-19T16:13:31Z

    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