Upper limit arguments of pg_logical_slot_xxx_changes functions accept invalid values
Hi,
While reading the replication slot codes, I found a wrong assignment
in pg_logical_slot_get_changes_guts() function as follows.
if (PG_ARGISNULL(2))
upto_nchanges = InvalidXLogRecPtr;
else
upto_nchanges = PG_GETARG_INT32(2);
Since the upto_nchanges is an integer value we should set 0 meaning
unlimited instead of InvalidXLogRecPtr. Since InvalidXLogRecPtr is
actually 0 this function works fine so far. Also I surprised that
these functions accept to set negative values to upto_nchanges. The
upto_lsn argument is also the same; it also accepts an invalid lsn.
For safety maybe it's better to reject non-NULL invalid values.That
way, the behavior of these functions are consistent with what the
documentation says;
If upto_lsn is non-NULL, decoding will include only those
transactions which commit prior to the specified LSN. If upto_nchanges
is non-NULL, decoding will stop when the number of rows produced by
decoding exceeds the specified value.
Attached patch fixes them. Feedback is very welcome.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
fix_logical_slot_changes_funcs.patchapplication/octet-stream; name=fix_logical_slot_changes_funcs.patchDownload
diff --git a/contrib/test_decoding/expected/slot.out b/contrib/test_decoding/expected/slot.out
index 2737a8a..bf2bd0c 100644
--- a/contrib/test_decoding/expected/slot.out
+++ b/contrib/test_decoding/expected/slot.out
@@ -46,6 +46,11 @@ SELECT pg_drop_replication_slot('regression_slot_t');
ERROR: replication slot "regression_slot_t" does not exist
SELECT pg_drop_replication_slot('regression_slot_t2');
ERROR: replication slot "regression_slot_t2" does not exist
+-- error
+SELECT data FROM pg_logical_slot_get_changes('regression_slot_p', '0/0', NULL); -- invalid upto_lsn
+ERROR: invalid upper limit wal lsn
+SELECT data FROM pg_logical_slot_get_changes('regression_slot_p', NULL, 0); -- invalid upto_nchanges
+ERROR: argument of upto_nchanges must be greater than zero
-- permanent slot has survived
SELECT pg_drop_replication_slot('regression_slot_p');
pg_drop_replication_slot
diff --git a/contrib/test_decoding/sql/slot.sql b/contrib/test_decoding/sql/slot.sql
index 24cdf71..ba5c9a2 100644
--- a/contrib/test_decoding/sql/slot.sql
+++ b/contrib/test_decoding/sql/slot.sql
@@ -26,6 +26,10 @@ end';
SELECT pg_drop_replication_slot('regression_slot_t');
SELECT pg_drop_replication_slot('regression_slot_t2');
+-- error
+SELECT data FROM pg_logical_slot_get_changes('regression_slot_p', '0/0', NULL); -- invalid upto_lsn
+SELECT data FROM pg_logical_slot_get_changes('regression_slot_p', NULL, 0); -- invalid upto_nchanges
+
-- permanent slot has survived
SELECT pg_drop_replication_slot('regression_slot_p');
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index 54c25f1..0e457ec 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -154,14 +154,26 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
name = PG_GETARG_NAME(0);
if (PG_ARGISNULL(1))
- upto_lsn = InvalidXLogRecPtr;
+ upto_lsn = InvalidXLogRecPtr; /* unlimited */
else
+ {
upto_lsn = PG_GETARG_LSN(1);
+ if (XLogRecPtrIsInvalid(upto_lsn))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid upper limit wal lsn")));
+ }
if (PG_ARGISNULL(2))
- upto_nchanges = InvalidXLogRecPtr;
+ upto_nchanges = 0; /* unlimited */
else
+ {
upto_nchanges = PG_GETARG_INT32(2);
+ if (upto_nchanges <= 0)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("argument of upto_nchanges must be greater than zero")));
+ }
if (PG_ARGISNULL(3))
ereport(ERROR,
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
The error messages are nice, and I think will be a helpful feature. This patch does add them correctly and also tests them.
A few things I noticed, in the code, that don't really interfere with functionality are below:
In the `if (PG_ARGISNULL(1))` section:
I was wondering if creating an alias called something like InfinateInvalidXLogRecPtr might be better than adding a comment saying infinite.
In the `if (PG_ARGISNULL(2))` section:
I think '0' is being used as a magic number here and could probably have another #define (linked to a place to put it below). Also, it seems a little weird to take NULL and then just set upto_nchanges to '0' while disallowing '0' to be passed in as the argument. Maybe just allowing '0' as an input would be ok and useful to some people. I don't believe I would have noticed that if 'upto_nchanges = UnlimitedNchanges` instead of 'upto_nchanges = 0'.
https://doxygen.postgresql.org/xlogdefs_8h_source.html#l00028
On Thu, Jul 12, 2018 at 09:58:16AM +0900, Masahiko Sawada wrote:
If upto_lsn is non-NULL, decoding will include only those
transactions which commit prior to the specified LSN. If upto_nchanges
is non-NULL, decoding will stop when the number of rows produced by
decoding exceeds the specified value.
It is also possible to interpret a negative value as an equivalent to
infinite, no? That's how I read the documentation quote you are adding
here.
--
Michael
Thank you for comment!
On Fri, Jul 27, 2018 at 7:27 AM, Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Jul 12, 2018 at 09:58:16AM +0900, Masahiko Sawada wrote:
If upto_lsn is non-NULL, decoding will include only those
transactions which commit prior to the specified LSN. If upto_nchanges
is non-NULL, decoding will stop when the number of rows produced by
decoding exceeds the specified value.It is also possible to interpret a negative value as an equivalent to
infinite, no? That's how I read the documentation quote you are adding
here.
Given the meaning of upto_nchanges I would expect that the "non-NULL"
is a positive value but it's possible to interpret as you mentioned.
Maybe this patch should fix only the code setting InvalidXLogRecPtr to
upto_nchanges.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On Wed, Jul 11, 2018 at 8:58 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
While reading the replication slot codes, I found a wrong assignment
in pg_logical_slot_get_changes_guts() function as follows.if (PG_ARGISNULL(2))
upto_nchanges = InvalidXLogRecPtr;
else
upto_nchanges = PG_GETARG_INT32(2);Since the upto_nchanges is an integer value we should set 0 meaning
unlimited instead of InvalidXLogRecPtr. Since InvalidXLogRecPtr is
actually 0 this function works fine so far.
If somebody changes InvalidXLogRecPtr to (uint64)-1, then it breaks as
the code is written. On the other hand, if somebody reverted
0ab9d1c4b31622e9176472b4276f3e9831e3d6ba, it would keep working as
written but break under your proposal.
I am not prepared to spend much time arguing about it, but I think we
should just leave this the way it is.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Sat, Jul 28, 2018 at 2:00 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Jul 11, 2018 at 8:58 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
While reading the replication slot codes, I found a wrong assignment
in pg_logical_slot_get_changes_guts() function as follows.if (PG_ARGISNULL(2))
upto_nchanges = InvalidXLogRecPtr;
else
upto_nchanges = PG_GETARG_INT32(2);Since the upto_nchanges is an integer value we should set 0 meaning
unlimited instead of InvalidXLogRecPtr. Since InvalidXLogRecPtr is
actually 0 this function works fine so far.If somebody changes InvalidXLogRecPtr to (uint64)-1, then it breaks as
the code is written. On the other hand, if somebody reverted
0ab9d1c4b31622e9176472b4276f3e9831e3d6ba, it would keep working as
written but break under your proposal.
I might be missing something but I think the setting either 0 or
negative values to it solves this problem. Since the upto_nchanges is
int32 we cannot build if somebody reverted
0ab9d1c4b31622e9176472b4276f3e9831e3d6ba.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
On Tue, Aug 14, 2018 at 10:00:49AM +0900, Masahiko Sawada wrote:
I might be missing something but I think the setting either 0 or
negative values to it solves this problem. Since the upto_nchanges is
int32 we cannot build if somebody reverted
0ab9d1c4b31622e9176472b4276f3e9831e3d6ba.
I don't link that we should change the existing behavior either which is
here for a couple of releases already, so let's mark the patch as
returned with feedback.
--
Michael