Thread: pg_stat_get_replication_slot() marked not strict, crashes
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
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
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
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
Attachment
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.
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
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
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/
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
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.