Upper limit arguments of pg_logical_slot_xxx_changes functions accept invalid values

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