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
Attachments:
0001-pg_stat_get_replication_slot-NULL-handling_Head.patchtext/x-patch; charset=US-ASCII; name=0001-pg_stat_get_replication_slot-NULL-handling_Head.patchDownload
From 578f8f51e7213b22152db7447d94f7fd48bf3893 Mon Sep 17 00:00:00 2001
From: Vigneshwaran C <vignesh21@gmail.com>
Date: Sun, 27 Mar 2022 11:30:09 +0530
Subject: [PATCH] pg_stat_get_replication_slot NULL handling.
Set isstrict to true for pg_stat_get_replication_slot function.
---
src/include/catalog/pg_proc.dat | 2 +-
src/test/regress/expected/misc_functions.out | 9 +++++++++
src/test/regress/sql/misc_functions.sql | 5 +++++
3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 699bd0aa3e..c14ccccdf5 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5370,7 +5370,7 @@
proargnames => '{pid,status,receive_start_lsn,receive_start_tli,written_lsn,flushed_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,sender_host,sender_port,conninfo}',
prosrc => 'pg_stat_get_wal_receiver' },
{ oid => '6169', descr => 'statistics: information about replication slot',
- proname => 'pg_stat_get_replication_slot', proisstrict => 'f', provolatile => 's',
+ proname => 'pg_stat_get_replication_slot', proisstrict => 't', provolatile => 's',
proparallel => 'r', prorettype => 'record', proargtypes => 'text',
proallargtypes => '{text,text,int8,int8,int8,int8,int8,int8,int8,int8,timestamptz}',
proargmodes => '{i,o,o,o,o,o,o,o,o,o,o}',
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index 01d1ad0b9a..900e89342c 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -466,6 +466,15 @@ SELECT has_function_privilege('regress_slot_dir_funcs',
(1 row)
DROP ROLE regress_slot_dir_funcs;
+--
+-- Test NULL handling of pg_stat_get_replication_slot
+--
+SELECT pg_stat_get_replication_slot(NULL);
+ pg_stat_get_replication_slot
+------------------------------
+
+(1 row)
+
--
-- Test adding a support function to a subject function
--
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index 072fc36a1f..34895a58c1 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -161,6 +161,11 @@ SELECT has_function_privilege('regress_slot_dir_funcs',
'pg_ls_replslotdir(text)', 'EXECUTE');
DROP ROLE regress_slot_dir_funcs;
+--
+-- Test NULL handling of pg_stat_get_replication_slot
+--
+SELECT pg_stat_get_replication_slot(NULL);
+
--
-- Test adding a support function to a subject function
--
--
2.32.0
0001-pg_stat_get_replication_slot-NULL-handling_PG14.patchtext/x-patch; charset=US-ASCII; name=0001-pg_stat_get_replication_slot-NULL-handling_PG14.patchDownload
From a5ff5cadf5bb386bc3b32b7c8340718ce44fdc74 Mon Sep 17 00:00:00 2001
From: Vigneshwaran C <vignesh21@gmail.com>
Date: Sun, 27 Mar 2022 11:47:05 +0530
Subject: [PATCH] pg_stat_get_replication_slot NULL handling.
Passing NULL to pg_stat_get_replication_slot crashes. Set isstrict to true for
pg_stat_get_replication_slot function to handle it.
---
src/include/catalog/pg_proc.dat | 2 +-
src/test/regress/expected/misc_functions.out | 8 ++++++++
src/test/regress/sql/misc_functions.sql | 5 +++++
3 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 79669bf5a2..4a1f0fe0ef 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5310,7 +5310,7 @@
proargnames => '{pid,status,receive_start_lsn,receive_start_tli,written_lsn,flushed_lsn,received_tli,last_msg_send_time,last_msg_receipt_time,latest_end_lsn,latest_end_time,slot_name,sender_host,sender_port,conninfo}',
prosrc => 'pg_stat_get_wal_receiver' },
{ oid => '6169', descr => 'statistics: information about replication slot',
- proname => 'pg_stat_get_replication_slot', prorows => '1', proisstrict => 'f',
+ proname => 'pg_stat_get_replication_slot', prorows => '1', proisstrict => 't',
proretset => 't', provolatile => 's', proparallel => 'r',
prorettype => 'record', proargtypes => 'text',
proallargtypes => '{text,text,int8,int8,int8,int8,int8,int8,int8,int8,timestamptz}',
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index e845042d38..3883aebdfe 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -214,6 +214,14 @@ select count(*) > 0 from
t
(1 row)
+--
+-- Test NULL handling of pg_stat_get_replication_slot
+--
+SELECT pg_stat_get_replication_slot(NULL);
+ pg_stat_get_replication_slot
+------------------------------
+(0 rows)
+
--
-- Test adding a support function to a subject function
--
diff --git a/src/test/regress/sql/misc_functions.sql b/src/test/regress/sql/misc_functions.sql
index a398349afc..5029f5d931 100644
--- a/src/test/regress/sql/misc_functions.sql
+++ b/src/test/regress/sql/misc_functions.sql
@@ -69,6 +69,11 @@ select count(*) > 0 from
where spcname = 'pg_default') pts
join pg_database db on pts.pts = db.oid;
+--
+-- Test NULL handling of pg_stat_get_replication_slot
+--
+SELECT pg_stat_get_replication_slot(NULL);
+
--
-- Test adding a support function to a subject function
--
--
2.32.0
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.