pgsql: Allow users to limit storage reserved by replication slots

On 2020-Apr-07, Alvaro Herrera wrote:

src/test/recovery/t/019_replslot_limit.pl | 217 +++++++++++++++++++++++++

I fixed the perlcritic complaint from buildfarm member crake, but
there's a new one in francolin:

# Failed test 'check that the slot state changes to "reserved"'
# at t/019_replslot_limit.pl line 125.
# got: '0/15000D8|reserved|216 bytes'
# expected: '0/1500000|reserved|216 bytes'

# Failed test 'check that the slot state changes to "lost"'
# at t/019_replslot_limit.pl line 135.
# got: '0/15000D8|lost|t'
# expected: '0/1500000|lost|t'
# Looks like you failed 2 tests of 13.
[23:07:28] t/019_replslot_limit.pl ..............

where the Perl code is:

$start_lsn = $node_master->lsn('write');
$node_master->wait_for_catchup($node_standby, 'replay', $start_lsn);
$node_standby->stop;

# Advance WAL again without checkpoint, reducing remain by 6 MB.
advance_wal($node_master, 6);

# Slot gets into 'reserved' state
$result = $node_master->safe_psql('postgres', "SELECT restart_lsn, wal_status, pg_size_pretty(restart_lsn - min_safe_lsn) as remain FROM pg_replication_slots WHERE slot_name = 'rep1'");
is($result, "$start_lsn|reserved|216 bytes", 'check that the slot state changes to "reserved"');

0xD8 is 216, so this seems to be saying that the checkpoint record was
skipped by the restart_lsn. I'm not clear exactly why that happened ...
is this saying that a checkpoint occurred?

One easy fix would be to remove the "restart_lsn" output column from the
query, but do we lose test specificity? (I think the answer is no.)

However, even with that change, we're still testing that a checkpoint is
216 bytes ... in other words, whenever someone changes the definition of
struct CheckPoint, this test will fail. That seems unnecessary and
unfriendly. I'm not sure how to improve that without also removing that
column.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

I fixed the perlcritic complaint from buildfarm member crake, but
there's a new one in francolin:

Other buildfarm members are showing related-but-different failures.
I think this test is just plain unstable.

regards, tom lane

Hi,

On April 7, 2020 6:13:51 PM PDT, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

I fixed the perlcritic complaint from buildfarm member crake, but
there's a new one in francolin:

Other buildfarm members are showing related-but-different failures.
I think this test is just plain unstable.

I have not looked at the source, but the error messages show LSNs and bytes. I can't really imagine how that could be made stable.

Andres
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.

On Tue, Apr 07, 2020 at 07:10:07PM -0700, Andres Freund wrote:

I have not looked at the source, but the error messages show LSNs
and bytes. I can't really imagine how that could be made stable.

Another bad news is that this is page-size dependent. What if you
removed pg_size_pretty() and replaced it with a condition that returns
a boolean status in the result itself?
--
Michael

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

However, even with that change, we're still testing that a checkpoint is
216 bytes ... in other words, whenever someone changes the definition of
struct CheckPoint, this test will fail. That seems unnecessary and
unfriendly. I'm not sure how to improve that without also removing that
column.

I read florican's results as showing that sizeof(CheckPoint) is already
different on 32-bit machines than 64-bit; it's repeatably getting this:

# Failed test 'check that the slot state changes to "reserved"'
# at t/019_replslot_limit.pl line 125.
# got: '0/15000C0|reserved|192 bytes'
# expected: '0/15000C0|reserved|216 bytes'

This test case was *not* well thought out.

regards, tom lane