Upper limit arguments of pg_logical_slot_xxx_changes functions accept invalid values
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t39048psql -h localhost -U postgresBuilt from patchset v1 (message #1), July 28, 2026 at 05:28 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t39048_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t39048_1 && git checkout t39048_1Patchset v1 (message #1) is on t39048_1
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
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