public inbox for [email protected]  
help / color / mirror / Atom feed
Re: null iv parameter passed to combo_init()
6+ messages / 3 participants
[nested] [flat]

* Re: null iv parameter passed to combo_init()
@ 2022-01-10 02:45  Zhihong Yu <[email protected]>
  0 siblings, 2 replies; 6+ messages in thread

From: Zhihong Yu @ 2022-01-10 02:45 UTC (permalink / raw)
  To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers

On Sun, Jan 9, 2022 at 1:27 PM Zhihong Yu <[email protected]> wrote:

>
>
> On Sun, Jan 9, 2022 at 12:38 PM Zhihong Yu <[email protected]> wrote:
>
>>
>>
>> On Sun, Jan 9, 2022 at 8:48 AM Noah Misch <[email protected]> wrote:
>>
>>> On Sun, Jan 09, 2022 at 04:37:32AM -0800, Zhihong Yu wrote:
>>> > On Sat, Jan 8, 2022 at 11:32 PM Tom Lane <[email protected]> wrote:
>>> > > Noah Misch <[email protected]> writes:
>>> > > > On further thought, I would write it this way:
>>> > >
>>> > > > -             else
>>> > > > +             else if (ivlen != 0)
>>> > > >                       memcpy(ivbuf, iv, ivlen);
>>> > >
>>> > > FWIW, I liked the "ivlen > 0" formulation better.  They should be
>>> > > equivalent, because ivlen is unsigned, but it just seems like "> 0"
>>> > > is more natural.
>>>
>>> If I were considering the one code site in isolation, I'd pick "ivlen >
>>> 0".
>>> But of the four sites identified so far, three have signed length
>>> variables.
>>> Since we're likely to get more examples of this pattern, some signed and
>>> some
>>> unsigned, I'd rather use a style that does the optimal thing whether or
>>> not
>>> the variable is signed.  What do you think?
>>>
>>> > Patch v4 is attached.
>>>
>>> Does this pass the test procedure shown upthread?
>>>
>> Hi,
>> I installed gcc 4.9.3
>>
>> When I ran:
>> ./configure CFLAGS='-fsanitize=undefined
>> -fsanitize-undefined-trap-on-error'
>>
>> I saw:
>>
>> configure:3977: $? = 0
>> configure:3966: gcc -V >&5
>> gcc: error: unrecognized command line option '-V'
>> gcc: fatal error: no input files
>> compilation terminated.
>> configure:3977: $? = 1
>> configure:3966: gcc -qversion >&5
>> gcc: error: unrecognized command line option '-qversion'
>> gcc: fatal error: no input files
>> compilation terminated.
>> configure:3977: $? = 1
>> configure:3997: checking whether the C compiler works
>> configure:4019: gcc -fsanitize=undefined
>> -fsanitize-undefined-trap-on-error   conftest.c  >&5
>> gcc: error: unrecognized command line option
>> '-fsanitize-undefined-trap-on-error'
>> configure:4023: $? = 1
>> configure:4061: result: no
>>
>> I wonder if a higher version gcc is needed.
>>
>> FYI
>>
>
> After installing gcc-11, ./configure passed (with 0003-memcpy-null.patch).
> In the output of `make check-world`, I don't see `runtime error`.
> Though there was a crash (maybe specific to my machine):
>
> Core was generated by
> `/nfusr/dev-server/zyu/postgres/tmp_install/usr/local/pgsql/bin/postgres
> --singl'.
> Program terminated with signal SIGILL, Illegal instruction.
> #0  0x000000000050642d in write_item.cold ()
> Missing separate debuginfos, use: debuginfo-install
> glibc-2.17-325.el7_9.x86_64 nss-pam-ldapd-0.8.13-25.el7.x86_64
> sssd-client-1.16.5-10.el7_9.10.x86_64
> (gdb) bt
> #0  0x000000000050642d in write_item.cold ()
> #1  0x0000000000ba9d1b in write_relcache_init_file ()
> #2  0x0000000000bb58f7 in RelationCacheInitializePhase3 ()
> #3  0x0000000000bd5cb5 in InitPostgres ()
> #4  0x0000000000a0a9ea in PostgresMain ()
>
> FYI
>
Hi,
Earlier I was using devtoolset-11 which had an `Illegal instruction` error.

I compiled / installed gcc-11 from source (which took whole afternoon).
`make check-world` passed with patch v3.
In tmp_install/log/install.log, I saw:

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Werror=vla -Wendif-labels
-Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
-Wno-format-truncation -Wno-stringop-truncation -fsanitize=undefined
-fsanitize-undefined-trap-on-error -I../../src/port -DFRONTEND
-I../../src/include  -D_GNU_SOURCE   -c -o path.o path.c
rm -f libpgport.a

Cheers


^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: null iv parameter passed to combo_init()
@ 2022-01-10 23:34  Zhihong Yu <[email protected]>
  parent: Zhihong Yu <[email protected]>
  1 sibling, 0 replies; 6+ messages in thread

From: Zhihong Yu @ 2022-01-10 23:34 UTC (permalink / raw)
  To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers

On Sun, Jan 9, 2022 at 6:45 PM Zhihong Yu <[email protected]> wrote:

>
>
> On Sun, Jan 9, 2022 at 1:27 PM Zhihong Yu <[email protected]> wrote:
>
>>
>>
>> On Sun, Jan 9, 2022 at 12:38 PM Zhihong Yu <[email protected]> wrote:
>>
>>>
>>>
>>> On Sun, Jan 9, 2022 at 8:48 AM Noah Misch <[email protected]> wrote:
>>>
>>>> On Sun, Jan 09, 2022 at 04:37:32AM -0800, Zhihong Yu wrote:
>>>> > On Sat, Jan 8, 2022 at 11:32 PM Tom Lane <[email protected]> wrote:
>>>> > > Noah Misch <[email protected]> writes:
>>>> > > > On further thought, I would write it this way:
>>>> > >
>>>> > > > -             else
>>>> > > > +             else if (ivlen != 0)
>>>> > > >                       memcpy(ivbuf, iv, ivlen);
>>>> > >
>>>> > > FWIW, I liked the "ivlen > 0" formulation better.  They should be
>>>> > > equivalent, because ivlen is unsigned, but it just seems like "> 0"
>>>> > > is more natural.
>>>>
>>>> If I were considering the one code site in isolation, I'd pick "ivlen >
>>>> 0".
>>>> But of the four sites identified so far, three have signed length
>>>> variables.
>>>> Since we're likely to get more examples of this pattern, some signed
>>>> and some
>>>> unsigned, I'd rather use a style that does the optimal thing whether or
>>>> not
>>>> the variable is signed.  What do you think?
>>>>
>>>> > Patch v4 is attached.
>>>>
>>>> Does this pass the test procedure shown upthread?
>>>>
>>> Hi,
>>> I installed gcc 4.9.3
>>>
>>> When I ran:
>>> ./configure CFLAGS='-fsanitize=undefined
>>> -fsanitize-undefined-trap-on-error'
>>>
>>> I saw:
>>>
>>> configure:3977: $? = 0
>>> configure:3966: gcc -V >&5
>>> gcc: error: unrecognized command line option '-V'
>>> gcc: fatal error: no input files
>>> compilation terminated.
>>> configure:3977: $? = 1
>>> configure:3966: gcc -qversion >&5
>>> gcc: error: unrecognized command line option '-qversion'
>>> gcc: fatal error: no input files
>>> compilation terminated.
>>> configure:3977: $? = 1
>>> configure:3997: checking whether the C compiler works
>>> configure:4019: gcc -fsanitize=undefined
>>> -fsanitize-undefined-trap-on-error   conftest.c  >&5
>>> gcc: error: unrecognized command line option
>>> '-fsanitize-undefined-trap-on-error'
>>> configure:4023: $? = 1
>>> configure:4061: result: no
>>>
>>> I wonder if a higher version gcc is needed.
>>>
>>> FYI
>>>
>>
>> After installing gcc-11, ./configure passed (with 0003-memcpy-null.patch).
>> In the output of `make check-world`, I don't see `runtime error`.
>> Though there was a crash (maybe specific to my machine):
>>
>> Core was generated by
>> `/nfusr/dev-server/zyu/postgres/tmp_install/usr/local/pgsql/bin/postgres
>> --singl'.
>> Program terminated with signal SIGILL, Illegal instruction.
>> #0  0x000000000050642d in write_item.cold ()
>> Missing separate debuginfos, use: debuginfo-install
>> glibc-2.17-325.el7_9.x86_64 nss-pam-ldapd-0.8.13-25.el7.x86_64
>> sssd-client-1.16.5-10.el7_9.10.x86_64
>> (gdb) bt
>> #0  0x000000000050642d in write_item.cold ()
>> #1  0x0000000000ba9d1b in write_relcache_init_file ()
>> #2  0x0000000000bb58f7 in RelationCacheInitializePhase3 ()
>> #3  0x0000000000bd5cb5 in InitPostgres ()
>> #4  0x0000000000a0a9ea in PostgresMain ()
>>
>> FYI
>>
> Hi,
> Earlier I was using devtoolset-11 which had an `Illegal instruction` error.
>
> I compiled / installed gcc-11 from source (which took whole afternoon).
> `make check-world` passed with patch v3.
> In tmp_install/log/install.log, I saw:
>
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
> -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
> -Wno-format-truncation -Wno-stringop-truncation -fsanitize=undefined
> -fsanitize-undefined-trap-on-error -I../../src/port -DFRONTEND
> -I../../src/include  -D_GNU_SOURCE   -c -o path.o path.c
> rm -f libpgport.a
>

Hi, Noah:
Patch v3 passes `make check-world`

Can you take another look ?


^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: null iv parameter passed to combo_init()
@ 2022-01-14 04:09  Noah Misch <[email protected]>
  parent: Zhihong Yu <[email protected]>
  1 sibling, 1 reply; 6+ messages in thread

From: Noah Misch @ 2022-01-14 04:09 UTC (permalink / raw)
  To: Zhihong Yu <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers

On Sun, Jan 09, 2022 at 06:45:09PM -0800, Zhihong Yu wrote:
> On Sun, Jan 9, 2022 at 1:27 PM Zhihong Yu <[email protected]> wrote:
> > After installing gcc-11, ./configure passed (with 0003-memcpy-null.patch).
> > In the output of `make check-world`, I don't see `runtime error`.

That's expected.  With -fsanitize-undefined-trap-on-error, the program will
generate SIGILL when UBSan detects undefined behavior.  To get "runtime error"
messages in the postmaster log, drop -fsanitize-undefined-trap-on-error.  Both
ways of running the tests have uses.  -fsanitize-undefined-trap-on-error is
better when you think the code is clean, because a zero "make check-world"
exit status confirms the code is clean.  Once you know the code is unclean in
some way, -fsanitize-undefined-trap-on-error is better for getting details.

> > Though there was a crash (maybe specific to my machine):
> >
> > Core was generated by
> > `/nfusr/dev-server/zyu/postgres/tmp_install/usr/local/pgsql/bin/postgres
> > --singl'.
> > Program terminated with signal SIGILL, Illegal instruction.
> > #0  0x000000000050642d in write_item.cold ()
> > Missing separate debuginfos, use: debuginfo-install
> > glibc-2.17-325.el7_9.x86_64 nss-pam-ldapd-0.8.13-25.el7.x86_64
> > sssd-client-1.16.5-10.el7_9.10.x86_64
> > (gdb) bt
> > #0  0x000000000050642d in write_item.cold ()
> > #1  0x0000000000ba9d1b in write_relcache_init_file ()
> > #2  0x0000000000bb58f7 in RelationCacheInitializePhase3 ()
> > #3  0x0000000000bd5cb5 in InitPostgres ()
> > #4  0x0000000000a0a9ea in PostgresMain ()

That is UBSan detecting undefined behavior.  A successful patch version will
fix write_item(), among many other places that are currently making
check-world fail.  I get the same when testing your v5 under "gcc (Debian
11.2.0-13) 11.2.0".  I used the same host as buildfarm member thorntail, and I
configured like this:

  ./configure -C --with-lz4 --prefix=$HOME/sw/nopath/pghead --enable-tap-tests --enable-debug --enable-depend --enable-cassert CC='ccache gcc-11 -fsanitize=undefined -fsanitize-undefined-trap-on-error' CFLAGS='-O2 -funwind-tables'

> Earlier I was using devtoolset-11 which had an `Illegal instruction` error.
> 
> I compiled / installed gcc-11 from source (which took whole afternoon).
> `make check-world` passed with patch v3.
> In tmp_install/log/install.log, I saw:
> 
> gcc -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
> -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard
> -Wno-format-truncation -Wno-stringop-truncation -fsanitize=undefined
> -fsanitize-undefined-trap-on-error -I../../src/port -DFRONTEND
> -I../../src/include  -D_GNU_SOURCE   -c -o path.o path.c
> rm -f libpgport.a

Perhaps this self-compiled gcc-11 is defective, being unable to detect the
instances of undefined behavior that other builds detect.  If so, use the
"devtoolset-11" gcc instead.  You're also building without optimization; that
might be the problem.






^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: null iv parameter passed to combo_init()
@ 2022-01-14 05:09  Zhihong Yu <[email protected]>
  parent: Noah Misch <[email protected]>
  0 siblings, 1 reply; 6+ messages in thread

From: Zhihong Yu @ 2022-01-14 05:09 UTC (permalink / raw)
  To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers

On Thu, Jan 13, 2022 at 8:09 PM Noah Misch <[email protected]> wrote:

> On Sun, Jan 09, 2022 at 06:45:09PM -0800, Zhihong Yu wrote:
> > On Sun, Jan 9, 2022 at 1:27 PM Zhihong Yu <[email protected]> wrote:
> > > After installing gcc-11, ./configure passed (with
> 0003-memcpy-null.patch).
> > > In the output of `make check-world`, I don't see `runtime error`.
>
> That's expected.  With -fsanitize-undefined-trap-on-error, the program will
> generate SIGILL when UBSan detects undefined behavior.  To get "runtime
> error"
> messages in the postmaster log, drop -fsanitize-undefined-trap-on-error.
> Both
> ways of running the tests have uses.  -fsanitize-undefined-trap-on-error is
> better when you think the code is clean, because a zero "make check-world"
> exit status confirms the code is clean.  Once you know the code is unclean
> in
> some way, -fsanitize-undefined-trap-on-error is better for getting details.
>
> > > Though there was a crash (maybe specific to my machine):
> > >
> > > Core was generated by
> > >
> `/nfusr/dev-server/zyu/postgres/tmp_install/usr/local/pgsql/bin/postgres
> > > --singl'.
> > > Program terminated with signal SIGILL, Illegal instruction.
> > > #0  0x000000000050642d in write_item.cold ()
> > > Missing separate debuginfos, use: debuginfo-install
> > > glibc-2.17-325.el7_9.x86_64 nss-pam-ldapd-0.8.13-25.el7.x86_64
> > > sssd-client-1.16.5-10.el7_9.10.x86_64
> > > (gdb) bt
> > > #0  0x000000000050642d in write_item.cold ()
> > > #1  0x0000000000ba9d1b in write_relcache_init_file ()
> > > #2  0x0000000000bb58f7 in RelationCacheInitializePhase3 ()
> > > #3  0x0000000000bd5cb5 in InitPostgres ()
> > > #4  0x0000000000a0a9ea in PostgresMain ()
>
> That is UBSan detecting undefined behavior.  A successful patch version
> will
> fix write_item(), among many other places that are currently making
> check-world fail.  I get the same when testing your v5 under "gcc (Debian
> 11.2.0-13) 11.2.0".  I used the same host as buildfarm member thorntail,
> and I
> configured like this:
>
>   ./configure -C --with-lz4 --prefix=$HOME/sw/nopath/pghead
> --enable-tap-tests --enable-debug --enable-depend --enable-cassert
> CC='ccache gcc-11 -fsanitize=undefined -fsanitize-undefined-trap-on-error'
> CFLAGS='-O2 -funwind-tables'
>
> > Earlier I was using devtoolset-11 which had an `Illegal instruction`
> error.
> >
> > I compiled / installed gcc-11 from source (which took whole afternoon).
> > `make check-world` passed with patch v3.
> > In tmp_install/log/install.log, I saw:
> >
> > gcc -Wall -Wmissing-prototypes -Wpointer-arith
> > -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> > -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
> > -Wformat-security -fno-strict-aliasing -fwrapv
> -fexcess-precision=standard
> > -Wno-format-truncation -Wno-stringop-truncation -fsanitize=undefined
> > -fsanitize-undefined-trap-on-error -I../../src/port -DFRONTEND
> > -I../../src/include  -D_GNU_SOURCE   -c -o path.o path.c
> > rm -f libpgport.a
>
> Perhaps this self-compiled gcc-11 is defective, being unable to detect the
> instances of undefined behavior that other builds detect.  If so, use the
> "devtoolset-11" gcc instead.  You're also building without optimization;
> that
> might be the problem.
>

I tried both locally built gcc-11 and devtoolset-11 with configure command
copied from above.
`make world` failed in both cases with:

performing post-bootstrap initialization ... sh: line 1: 24714 Illegal
instruction     (core dumped)
".../postgres/tmp_install/.../postgres/bin/postgres" --single -F -O -j -c
search_path=pg_catalog -c exit_on_error=true -c log_checkpoints=false
template1 > /dev/null
child process exited with exit code 132

#0  0x000000000050a8d6 in write_item (data=<optimized out>, len=<optimized
out>, fp=<optimized out>) at relcache.c:6471
#1  0x0000000000c33273 in write_relcache_init_file (shared=true) at
relcache.c:6368
#2  0x0000000000c33c50 in RelationCacheInitializePhase3 () at
relcache.c:4220
#3  0x0000000000c55825 in InitPostgres (in_dbname=<optimized out>,
dboid=3105442800, username=<optimized out>, useroid=<optimized out>,
out_dbname=0x0, override_allow_connections=<optimized out>) at
postinit.c:1014

FYI


^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* Re: null iv parameter passed to combo_init()
@ 2022-01-14 05:12  Zhihong Yu <[email protected]>
  parent: Zhihong Yu <[email protected]>
  0 siblings, 0 replies; 6+ messages in thread

From: Zhihong Yu @ 2022-01-14 05:12 UTC (permalink / raw)
  To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers

On Thu, Jan 13, 2022 at 9:09 PM Zhihong Yu <[email protected]> wrote:

>
>
> On Thu, Jan 13, 2022 at 8:09 PM Noah Misch <[email protected]> wrote:
>
>> On Sun, Jan 09, 2022 at 06:45:09PM -0800, Zhihong Yu wrote:
>> > On Sun, Jan 9, 2022 at 1:27 PM Zhihong Yu <[email protected]> wrote:
>> > > After installing gcc-11, ./configure passed (with
>> 0003-memcpy-null.patch).
>> > > In the output of `make check-world`, I don't see `runtime error`.
>>
>> That's expected.  With -fsanitize-undefined-trap-on-error, the program
>> will
>> generate SIGILL when UBSan detects undefined behavior.  To get "runtime
>> error"
>> messages in the postmaster log, drop -fsanitize-undefined-trap-on-error.
>> Both
>> ways of running the tests have uses.  -fsanitize-undefined-trap-on-error
>> is
>> better when you think the code is clean, because a zero "make check-world"
>> exit status confirms the code is clean.  Once you know the code is
>> unclean in
>> some way, -fsanitize-undefined-trap-on-error is better for getting
>> details.
>>
>> > > Though there was a crash (maybe specific to my machine):
>> > >
>> > > Core was generated by
>> > >
>> `/nfusr/dev-server/zyu/postgres/tmp_install/usr/local/pgsql/bin/postgres
>> > > --singl'.
>> > > Program terminated with signal SIGILL, Illegal instruction.
>> > > #0  0x000000000050642d in write_item.cold ()
>> > > Missing separate debuginfos, use: debuginfo-install
>> > > glibc-2.17-325.el7_9.x86_64 nss-pam-ldapd-0.8.13-25.el7.x86_64
>> > > sssd-client-1.16.5-10.el7_9.10.x86_64
>> > > (gdb) bt
>> > > #0  0x000000000050642d in write_item.cold ()
>> > > #1  0x0000000000ba9d1b in write_relcache_init_file ()
>> > > #2  0x0000000000bb58f7 in RelationCacheInitializePhase3 ()
>> > > #3  0x0000000000bd5cb5 in InitPostgres ()
>> > > #4  0x0000000000a0a9ea in PostgresMain ()
>>
>> That is UBSan detecting undefined behavior.  A successful patch version
>> will
>> fix write_item(), among many other places that are currently making
>> check-world fail.  I get the same when testing your v5 under "gcc (Debian
>> 11.2.0-13) 11.2.0".  I used the same host as buildfarm member thorntail,
>> and I
>> configured like this:
>>
>>   ./configure -C --with-lz4 --prefix=$HOME/sw/nopath/pghead
>> --enable-tap-tests --enable-debug --enable-depend --enable-cassert
>> CC='ccache gcc-11 -fsanitize=undefined -fsanitize-undefined-trap-on-error'
>> CFLAGS='-O2 -funwind-tables'
>>
>> > Earlier I was using devtoolset-11 which had an `Illegal instruction`
>> error.
>> >
>> > I compiled / installed gcc-11 from source (which took whole afternoon).
>> > `make check-world` passed with patch v3.
>> > In tmp_install/log/install.log, I saw:
>> >
>> > gcc -Wall -Wmissing-prototypes -Wpointer-arith
>> > -Wdeclaration-after-statement -Werror=vla -Wendif-labels
>> > -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wcast-function-type
>> > -Wformat-security -fno-strict-aliasing -fwrapv
>> -fexcess-precision=standard
>> > -Wno-format-truncation -Wno-stringop-truncation -fsanitize=undefined
>> > -fsanitize-undefined-trap-on-error -I../../src/port -DFRONTEND
>> > -I../../src/include  -D_GNU_SOURCE   -c -o path.o path.c
>> > rm -f libpgport.a
>>
>> Perhaps this self-compiled gcc-11 is defective, being unable to detect the
>> instances of undefined behavior that other builds detect.  If so, use the
>> "devtoolset-11" gcc instead.  You're also building without optimization;
>> that
>> might be the problem.
>>
>
> I tried both locally built gcc-11 and devtoolset-11 with configure command
> copied from above.
> `make world` failed in both cases with:
>
> performing post-bootstrap initialization ... sh: line 1: 24714 Illegal
> instruction     (core dumped)
> ".../postgres/tmp_install/.../postgres/bin/postgres" --single -F -O -j -c
> search_path=pg_catalog -c exit_on_error=true -c log_checkpoints=false
> template1 > /dev/null
> child process exited with exit code 132
>
> #0  0x000000000050a8d6 in write_item (data=<optimized out>, len=<optimized
> out>, fp=<optimized out>) at relcache.c:6471
> #1  0x0000000000c33273 in write_relcache_init_file (shared=true) at
> relcache.c:6368
> #2  0x0000000000c33c50 in RelationCacheInitializePhase3 () at
> relcache.c:4220
> #3  0x0000000000c55825 in InitPostgres (in_dbname=<optimized out>,
> dboid=3105442800, username=<optimized out>, useroid=<optimized out>,
> out_dbname=0x0, override_allow_connections=<optimized out>) at
> postinit.c:1014
>
> FYI
>
Hi,
I forgot to mention that patch v5 was included during the experiment.

Cheers


^ permalink  raw  reply  [nested|flat] 6+ messages in thread

* [PATCH v12 2/7] Row pattern recognition patch (parse/analysis).
@ 2023-12-04 11:23  Tatsuo Ishii <[email protected]>
  0 siblings, 0 replies; 6+ messages in thread

From: Tatsuo Ishii @ 2023-12-04 11:23 UTC (permalink / raw)

---
 src/backend/parser/parse_agg.c    |   7 +
 src/backend/parser/parse_clause.c | 278 +++++++++++++++++++++++++++++-
 src/backend/parser/parse_expr.c   |   4 +
 src/backend/parser/parse_func.c   |   3 +
 4 files changed, 291 insertions(+), 1 deletion(-)

diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index 9bbad33fbd..28fb5e0d71 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -575,6 +575,10 @@ check_agglevels_and_constraints(ParseState *pstate, Node *expr)
 			errkind = true;
 			break;
 
+		case EXPR_KIND_RPR_DEFINE:
+			errkind = true;
+			break;
+
 			/*
 			 * There is intentionally no default: case here, so that the
 			 * compiler will warn if we add a new ParseExprKind without
@@ -964,6 +968,9 @@ transformWindowFuncCall(ParseState *pstate, WindowFunc *wfunc,
 		case EXPR_KIND_CYCLE_MARK:
 			errkind = true;
 			break;
+		case EXPR_KIND_RPR_DEFINE:
+			errkind = true;
+			break;
 
 			/*
 			 * There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 334b9b42bd..c5d3c10683 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -100,7 +100,10 @@ static WindowClause *findWindowClause(List *wclist, const char *name);
 static Node *transformFrameOffset(ParseState *pstate, int frameOptions,
 								  Oid rangeopfamily, Oid rangeopcintype, Oid *inRangeFunc,
 								  Node *clause);
-
+static void transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist);
+static List *transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist);
+static void transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
+static List *transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef);
 
 /*
  * transformFromClause -
@@ -2950,6 +2953,10 @@ transformWindowDefinitions(ParseState *pstate,
 											 rangeopfamily, rangeopcintype,
 											 &wc->endInRangeFunc,
 											 windef->endOffset);
+
+		/* Process Row Pattern Recognition related clauses */
+		transformRPR(pstate, wc, windef, targetlist);
+
 		wc->runCondition = NIL;
 		wc->winref = winref;
 
@@ -3815,3 +3822,272 @@ transformFrameOffset(ParseState *pstate, int frameOptions,
 
 	return node;
 }
+
+/*
+ * transformRPR
+ *		Process Row Pattern Recognition related clauses
+ */
+static void
+transformRPR(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist)
+{
+	/*
+	 * Window definition exists?
+	 */
+	if (windef == NULL)
+		return;
+
+	/*
+	 * Row Pattern Common Syntax clause exists?
+	 */
+	if (windef->rpCommonSyntax == NULL)
+		return;
+
+	/* Check Frame option. Frame must start at current row */
+	if ((wc->frameOptions & FRAMEOPTION_START_CURRENT_ROW) == 0)
+		ereport(ERROR,
+				(errcode(ERRCODE_SYNTAX_ERROR),
+				 errmsg("FRAME must start at current row when row patttern recognition is used")));
+
+	/* Transform AFTER MACH SKIP TO clause */
+	wc->rpSkipTo = windef->rpCommonSyntax->rpSkipTo;
+
+	/* Transform SEEK or INITIAL clause */
+	wc->initial = windef->rpCommonSyntax->initial;
+
+	/* Transform DEFINE clause into list of TargetEntry's */
+	wc->defineClause = transformDefineClause(pstate, wc, windef, targetlist);
+
+	/* Check PATTERN clause and copy to patternClause */
+	transformPatternClause(pstate, wc, windef);
+
+	/* Transform MEASURE clause */
+	transformMeasureClause(pstate, wc, windef);
+}
+
+/*
+ * transformDefineClause Process DEFINE clause and transform ResTarget into
+ *		list of TargetEntry.
+ *
+ * XXX we only support column reference in row pattern definition search
+ * condition, e.g. "price". <row pattern definition variable name>.<column
+ * reference> is not supported, e.g. "A.price".
+ */
+static List *
+transformDefineClause(ParseState *pstate, WindowClause *wc, WindowDef *windef, List **targetlist)
+{
+	/* DEFINE variable name initials */
+	static	char	*defineVariableInitials = "abcdefghijklmnopqrstuvwxyz";
+
+	ListCell		*lc, *l;
+	ResTarget		*restarget, *r;
+	List			*restargets;
+	List			*defineClause;
+	char			*name;
+	int				initialLen;
+	int				i;
+
+	/*
+	 * If Row Definition Common Syntax exists, DEFINE clause must exist.
+	 * (the raw parser should have already checked it.)
+	 */
+	Assert(windef->rpCommonSyntax->rpDefs != NULL);
+
+	/*
+	 * Check and add "A AS A IS TRUE" if pattern variable is missing in DEFINE
+	 * per the SQL standard.
+	 */
+	restargets = NIL;
+	foreach(lc, windef->rpCommonSyntax->rpPatterns)
+	{
+		A_Expr	*a;
+		bool	found = false;
+
+		if (!IsA(lfirst(lc), A_Expr))
+			ereport(ERROR,
+					errmsg("node type is not A_Expr"));
+
+		a = (A_Expr *)lfirst(lc);
+		name = strVal(a->lexpr);
+
+		foreach(l, windef->rpCommonSyntax->rpDefs)
+		{
+			restarget = (ResTarget *)lfirst(l);
+
+			if (!strcmp(restarget->name, name))
+			{
+				found = true;
+				break;
+			}
+		}
+
+		if (!found)
+		{
+			/*
+			 * "name" is missing. So create "name AS name IS TRUE" ResTarget
+			 * node and add it to the temporary list.
+			 */
+			A_Const	   *n;
+
+			restarget = makeNode(ResTarget);
+			n = makeNode(A_Const);
+			n->val.boolval.type = T_Boolean;
+			n->val.boolval.boolval = true;
+			n->location = -1;
+			restarget->name = pstrdup(name);
+			restarget->indirection = NIL;
+			restarget->val = (Node *)n;
+			restarget->location = -1;
+			restargets = lappend((List *)restargets, restarget);
+		}
+	}
+
+	if (list_length(restargets) >= 1)
+	{
+		/* add missing DEFINEs */
+		windef->rpCommonSyntax->rpDefs = list_concat(windef->rpCommonSyntax->rpDefs,
+													 restargets);
+		list_free(restargets);
+	}
+
+	/*
+	 * Check for duplicate row pattern definition variables.  The standard
+	 * requires that no two row pattern definition variable names shall be
+	 * equivalent.
+	 */
+	restargets = NIL;
+	foreach(lc, windef->rpCommonSyntax->rpDefs)
+	{
+		restarget = (ResTarget *)lfirst(lc);
+		name = restarget->name;
+
+		/*
+		 * Add DEFINE expression (Restarget->val) to the targetlist as a
+		 * TargetEntry if it does not exist yet. Planner will add the column
+		 * ref var node to the outer plan's target list later on. This makes
+		 * DEFINE expression could access the outer tuple while evaluating
+		 * PATTERN.
+		 *
+		 * XXX: adding whole expressions of DEFINE to the plan.targetlist is
+		 * not so good, because it's not necessary to evalute the expression
+		 * in the target list while running the plan. We should extract the
+		 * var nodes only then add them to the plan.targetlist.
+		 */
+		findTargetlistEntrySQL99(pstate, (Node *)restarget->val, targetlist, EXPR_KIND_RPR_DEFINE);
+
+		/*
+		 * Make sure that the row pattern definition search condition is a
+		 * boolean expression.
+		 */
+		transformWhereClause(pstate, restarget->val,
+							 EXPR_KIND_RPR_DEFINE, "DEFINE");
+
+		foreach(l, restargets)
+		{
+			char		*n;
+
+			r = (ResTarget *) lfirst(l);
+			n = r->name;
+
+			if (!strcmp(n, name))
+				ereport(ERROR,
+						(errcode(ERRCODE_SYNTAX_ERROR),
+						 errmsg("row pattern definition variable name \"%s\" appears more than once in DEFINE clause",
+								name),
+						 parser_errposition(pstate, exprLocation((Node *)r))));
+		}
+		restargets = lappend(restargets, restarget);
+	}
+	list_free(restargets);
+
+	/*
+	 * Create list of row pattern DEFINE variable name's initial.
+	 * We assign [a-z] to them (up to 26 variable names are allowed).
+	 */
+	restargets = NIL;
+	i = 0;
+	initialLen = strlen(defineVariableInitials);
+
+	foreach(lc, windef->rpCommonSyntax->rpDefs)
+	{
+		char	initial[2];
+
+		restarget = (ResTarget *)lfirst(lc);
+		name = restarget->name;
+
+		if (i >= initialLen)
+		{
+			ereport(ERROR,
+					(errcode(ERRCODE_SYNTAX_ERROR),
+					 errmsg("number of row pattern definition variable names exceeds %d", initialLen),
+					 parser_errposition(pstate, exprLocation((Node *)restarget))));
+		}
+		initial[0] = defineVariableInitials[i++];
+		initial[1] = '\0';
+		wc->defineInitial = lappend(wc->defineInitial, makeString(pstrdup(initial)));
+	}
+
+	defineClause = transformTargetList(pstate, windef->rpCommonSyntax->rpDefs,
+							   EXPR_KIND_RPR_DEFINE);
+
+	/* mark column origins */
+	markTargetListOrigins(pstate, defineClause);
+
+	/* mark all nodes in the DEFINE clause tree with collation information */
+	assign_expr_collations(pstate, (Node *)defineClause);
+
+	return defineClause;
+}
+
+/*
+ * transformPatternClause
+ *		Process PATTERN clause and return PATTERN clause in the raw parse tree
+ */
+static void
+transformPatternClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+	ListCell	*lc;
+
+	/*
+	 * Row Pattern Common Syntax clause exists?
+	 */
+	if (windef->rpCommonSyntax == NULL)
+		return;
+
+	wc->patternVariable = NIL;
+	wc->patternRegexp = NIL;
+	foreach(lc, windef->rpCommonSyntax->rpPatterns)
+	{
+		A_Expr	*a;
+		char	*name;
+		char	*regexp;
+
+		if (!IsA(lfirst(lc), A_Expr))
+			ereport(ERROR,
+					errmsg("node type is not A_Expr"));
+
+		a = (A_Expr *)lfirst(lc);
+		name = strVal(a->lexpr);
+
+		wc->patternVariable = lappend(wc->patternVariable, makeString(pstrdup(name)));
+		regexp = strVal(lfirst(list_head(a->name)));
+		wc->patternRegexp = lappend(wc->patternRegexp, makeString(pstrdup(regexp)));
+	}
+}
+
+/*
+ * transformMeasureClause
+ *		Process MEASURE clause
+ *	XXX MEASURE clause is not supported yet
+ */
+static List *
+transformMeasureClause(ParseState *pstate, WindowClause *wc, WindowDef *windef)
+{
+	if (windef->rowPatternMeasures == NIL)
+		return NIL;
+
+	ereport(ERROR,
+			(errcode(ERRCODE_SYNTAX_ERROR),
+			 errmsg("%s","MEASURE clause is not supported yet"),
+			 parser_errposition(pstate, exprLocation((Node *)windef->rowPatternMeasures))));
+	return NIL;
+}
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 64c582c344..18b58ac263 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -557,6 +557,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 		case EXPR_KIND_COPY_WHERE:
 		case EXPR_KIND_GENERATED_COLUMN:
 		case EXPR_KIND_CYCLE_MARK:
+		case EXPR_KIND_RPR_DEFINE:
 			/* okay */
 			break;
 
@@ -1770,6 +1771,7 @@ transformSubLink(ParseState *pstate, SubLink *sublink)
 		case EXPR_KIND_VALUES:
 		case EXPR_KIND_VALUES_SINGLE:
 		case EXPR_KIND_CYCLE_MARK:
+		case EXPR_KIND_RPR_DEFINE:
 			/* okay */
 			break;
 		case EXPR_KIND_CHECK_CONSTRAINT:
@@ -3149,6 +3151,8 @@ ParseExprKindName(ParseExprKind exprKind)
 			return "GENERATED AS";
 		case EXPR_KIND_CYCLE_MARK:
 			return "CYCLE";
+		case EXPR_KIND_RPR_DEFINE:
+			return "DEFINE";
 
 			/*
 			 * There is intentionally no default: case here, so that the
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 6c29471bb3..086431f91b 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -2656,6 +2656,9 @@ check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
 		case EXPR_KIND_CYCLE_MARK:
 			errkind = true;
 			break;
+		case EXPR_KIND_RPR_DEFINE:
+			errkind = true;
+			break;
 
 			/*
 			 * There is intentionally no default: case here, so that the
-- 
2.25.1


----Next_Part(Fri_Dec__8_10_16_13_2023_489)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="v12-0003-Row-pattern-recognition-patch-planner.patch"



^ permalink  raw  reply  [nested|flat] 6+ messages in thread


end of thread, other threads:[~2023-12-04 11:23 UTC | newest]

Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-01-10 02:45 Re: null iv parameter passed to combo_init() Zhihong Yu <[email protected]>
2022-01-10 23:34 ` Zhihong Yu <[email protected]>
2022-01-14 04:09 ` Noah Misch <[email protected]>
2022-01-14 05:09   ` Zhihong Yu <[email protected]>
2022-01-14 05:12     ` Zhihong Yu <[email protected]>
2023-12-04 11:23 [PATCH v12 2/7] Row pattern recognition patch (parse/analysis). Tatsuo Ishii <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox