Thread: Upper limit arguments of pg_logical_slot_xxx_changes functions acceptinvalid values
Upper limit arguments of pg_logical_slot_xxx_changes functions acceptinvalid values
From
Masahiko Sawada
Date:
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
Attachment
Re: Upper limit arguments of pg_logical_slot_xxx_changes functionsaccept invalid values
From
Brian Faherty
Date:
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 commentsaying 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 passedin as the argument. Maybe just allowing '0' as an input would be ok and useful to some people. I don't believe I wouldhave noticed that if 'upto_nchanges = UnlimitedNchanges` instead of 'upto_nchanges = 0'. https://doxygen.postgresql.org/xlogdefs_8h_source.html#l00028
Re: Upper limit arguments of pg_logical_slot_xxx_changes functionsaccept invalid values
From
Michael Paquier
Date:
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
Attachment
Re: Upper limit arguments of pg_logical_slot_xxx_changes functionsaccept invalid values
From
Masahiko Sawada
Date:
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
Re: Upper limit arguments of pg_logical_slot_xxx_changes functionsaccept invalid values
From
Robert Haas
Date:
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
Re: Upper limit arguments of pg_logical_slot_xxx_changes functionsaccept invalid values
From
Masahiko Sawada
Date:
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
Re: Upper limit arguments of pg_logical_slot_xxx_changes functionsaccept invalid values
From
Michael Paquier
Date:
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