Horton

Thread

Commits

  1. Mark pg_stat_get_subscription_stats() strict.

  2. Fix NULL input behaviour of pg_stat_get_replication_slot().

  1. pg_stat_get_replication_slot() marked not strict, crashes

    Andres Freund <andres@anarazel.de> — 2022-03-26T21:24:32Z

    Hi,

I'm working to increase the test coverage of pgstat related stuff higher (for
the shared memory stats patch, of course).

"Accidentally" noticed that
  SELECT * FROM pg_stat_get_replication_slot(NULL);
crashes.  This is present in HEAD and 14.

I guess we'll have to add a code-level check in 14 to deal with this?


pg_stat_get_subscription_stats() also is wrongly marked. But at least in the
trivial cases just returns bogus results (for 0/InvalidOid). That's only in
HEAD, so easy to deal with.

The other functions returned by
  SELECT oid::regprocedure FROM pg_proc WHERE proname LIKE 'pg%stat%' AND pronargs > 0 AND NOT proisstrict;
look ok.


I wonder if we ought to make PG_GETARG_DATUM(n) assert that !PG_ARGISNULL(n)?
That'd perhaps make it easier to catch some of these...

It'd be nice to have a test in sanity check to just call each non-strict
function with NULL inputs automatically. But the potential side-effects
probably makes that not a realistic option?

Greetings,

Andres Freund

  2. Re: pg_stat_get_replication_slot() marked not strict, crashes

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-26T21:41:53Z

    Andres Freund <andres@anarazel.de> writes:
> I wonder if we ought to make PG_GETARG_DATUM(n) assert that !PG_ARGISNULL(n)?
> That'd perhaps make it easier to catch some of these...

Don't see the point; such cases will crash just fine without any
assert.  The problem is lack of test coverage ...

> It'd be nice to have a test in sanity check to just call each non-strict
> function with NULL inputs automatically. But the potential side-effects
> probably makes that not a realistic option?

... and as you say, brute force testing seems difficult.  I'm
particularly worried about multi-argument functions, as in
principle we'd need to check each argument separately, and cons
up something plausible to pass to the other arguments.

			regards, tom lane

  3. Re: pg_stat_get_replication_slot() marked not strict, crashes

    Andres Freund <andres@anarazel.de> — 2022-03-26T21:52:23Z

    Hi,

On 2022-03-26 17:41:53 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I wonder if we ought to make PG_GETARG_DATUM(n) assert that !PG_ARGISNULL(n)?
> > That'd perhaps make it easier to catch some of these...
> 
> Don't see the point; such cases will crash just fine without any
> assert.  The problem is lack of test coverage ...

Not reliably. Byval types typically won't crash, just do something
bogus. As e.g. in the case of pg_stat_get_subscription_stats(NULL) I found to
also be wrong upthread.

Greetings,

Andres Freund

  4. Re: pg_stat_get_replication_slot() marked not strict, crashes

    vignesh C <vignesh21@gmail.com> — 2022-03-27T06:29:34Z

    On Sun, Mar 27, 2022 at 2:54 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> I'm working to increase the test coverage of pgstat related stuff higher (for
> the shared memory stats patch, of course).
>
> "Accidentally" noticed that
>   SELECT * FROM pg_stat_get_replication_slot(NULL);
> crashes.  This is present in HEAD and 14.
>
> I guess we'll have to add a code-level check in 14 to deal with this?

This problem is reproducible in both PG14 & Head, changing isstrict
solves the problem. In PG14 should we also add a check in
pg_stat_get_replication_slot so that it can solve the problem for the
existing users who have already installed PG14 or will this be handled
automatically when upgrading to the new version.

Regards,
Vignesh

  5. Re: pg_stat_get_replication_slot() marked not strict, crashes

    Amit Kapila <amit.kapila16@gmail.com> — 2022-03-28T02:58:29Z

    On Sun, Mar 27, 2022 at 11:59 AM vignesh C <vignesh21@gmail.com> wrote:
>
> On Sun, Mar 27, 2022 at 2:54 AM Andres Freund <andres@anarazel.de> wrote:
> >
> > Hi,
> >
> > I'm working to increase the test coverage of pgstat related stuff higher (for
> > the shared memory stats patch, of course).
> >
> > "Accidentally" noticed that
> >   SELECT * FROM pg_stat_get_replication_slot(NULL);
> > crashes.  This is present in HEAD and 14.
> >
> > I guess we'll have to add a code-level check in 14 to deal with this?
>
> This problem is reproducible in both PG14 & Head, changing isstrict
> solves the problem. In PG14 should we also add a check in
> pg_stat_get_replication_slot so that it can solve the problem for the
> existing users who have already installed PG14 or will this be handled
> automatically when upgrading to the new version.
>

I am not sure if for 14 we can make a catalog change as that would
require catversion bump, so adding a code-level check as suggested by
Andres seems like a better option. Andres/Tom, any better ideas for
this?

Thanks for the patch but for HEAD, we also need handling and test for
pg_stat_get_subscription_stats. Considering this for HEAD, we can mark
both pg_stat_get_replication_slot and pg_stat_get_subscription_stats
as strict and in 14, we need to add a code-level check for
pg_stat_get_replication_slot.

-- 
With Regards,
Amit Kapila.

  6. Re: pg_stat_get_replication_slot() marked not strict, crashes

    Andres Freund <andres@anarazel.de> — 2022-03-28T04:09:29Z

    Hi,

On 2022-03-28 08:28:29 +0530, Amit Kapila wrote:
> I am not sure if for 14 we can make a catalog change as that would
> require catversion bump, so adding a code-level check as suggested by
> Andres seems like a better option. Andres/Tom, any better ideas for
> this?

I think we could do the catalog change too, so that future initdb's are marked
correctly. But we obviously do need the code-level check nevertheless.


> Thanks for the patch but for HEAD, we also need handling and test for
> pg_stat_get_subscription_stats. Considering this for HEAD, we can mark
> both pg_stat_get_replication_slot and pg_stat_get_subscription_stats
> as strict and in 14, we need to add a code-level check for
> pg_stat_get_replication_slot.

FWIW, I have a test for both, I was a bit "stuck" on where to put the
pg_stat_get_subscription_stats(NULL) test. I had put the
pg_stat_get_replication_slot(NULL) in contrib/test_decoding/sql/stats.sql
but pg_stat_get_subscription_stats() doesn't really fit there.  I think I'm
coming down to putting a section of such tests into src/test/regress/sql/stats.sql
instead. In the hope of preventing future such occurrances by encouraging
people to copy the test...

Greetings,

Andres Freund

  7. Re: pg_stat_get_replication_slot() marked not strict, crashes

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-28T04:17:42Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-03-28 08:28:29 +0530, Amit Kapila wrote:
>> I am not sure if for 14 we can make a catalog change as that would
>> require catversion bump, so adding a code-level check as suggested by
>> Andres seems like a better option. Andres/Tom, any better ideas for
>> this?

> I think we could do the catalog change too, so that future initdb's are marked
> correctly. But we obviously do need the code-level check nevertheless.

Yeah.  We have to install the C-level check, so I don't see any
point in changing the catalogs in back branches.  That'll create
confusion while not saving anything.

			regards, tom lane

  8. Re: pg_stat_get_replication_slot() marked not strict, crashes

    Masahiko Sawada <sawada.mshk@gmail.com> — 2022-03-28T04:42:47Z

    On Sun, Mar 27, 2022 at 6:52 AM Andres Freund <andres@anarazel.de> wrote:
>
> Hi,
>
> On 2022-03-26 17:41:53 -0400, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > I wonder if we ought to make PG_GETARG_DATUM(n) assert that !PG_ARGISNULL(n)?
> > > That'd perhaps make it easier to catch some of these...
> >
> > Don't see the point; such cases will crash just fine without any
> > assert.  The problem is lack of test coverage ...
>
> Not reliably. Byval types typically won't crash, just do something
> bogus. As e.g. in the case of pg_stat_get_subscription_stats(NULL) I found to
> also be wrong upthread.

Right. But it seems like we cannot simply add PG_ARGISNULL () to
PG_GETARG_DATUM(). There are some codes such as array_remove() and
array_replace() that call PG_GETARG_DATUM() and PG_ARGISNULL() and
pass these values to functions that do is-null check

Regards,

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

  9. Re: pg_stat_get_replication_slot() marked not strict, crashes

    Andres Freund <andres@anarazel.de> — 2022-03-28T04:54:12Z

    Hi,

On 2022-03-27 21:09:29 -0700, Andres Freund wrote:
> FWIW, I have a test for both, I was a bit "stuck" on where to put the
> pg_stat_get_subscription_stats(NULL) test. I had put the
> pg_stat_get_replication_slot(NULL) in contrib/test_decoding/sql/stats.sql
> but pg_stat_get_subscription_stats() doesn't really fit there.  I think I'm
> coming down to putting a section of such tests into src/test/regress/sql/stats.sql
> instead. In the hope of preventing future such occurrances by encouraging
> people to copy the test...

Pushed with tests there.

Vignesh, thanks for the patches! I already had something locally, should have
mentioned that...

Greetings,

Andres Freund

  10. Re: pg_stat_get_replication_slot() marked not strict, crashes

    Amit Kapila <amit.kapila16@gmail.com> — 2022-03-28T06:34:11Z

    On Mon, Mar 28, 2022 at 10:24 AM Andres Freund <andres@anarazel.de> wrote:
>
> On 2022-03-27 21:09:29 -0700, Andres Freund wrote:
> > FWIW, I have a test for both, I was a bit "stuck" on where to put the
> > pg_stat_get_subscription_stats(NULL) test. I had put the
> > pg_stat_get_replication_slot(NULL) in contrib/test_decoding/sql/stats.sql
> > but pg_stat_get_subscription_stats() doesn't really fit there.  I think I'm
> > coming down to putting a section of such tests into src/test/regress/sql/stats.sql
> > instead. In the hope of preventing future such occurrances by encouraging
> > people to copy the test...
>
> Pushed with tests there.
>

Thank you.

-- 
With Regards,
Amit Kapila.