Hi, While testing “[a850be2fe] Add max_retention_duration option to subscriptions”, I found a small problem where max_retention_duration accepts negative values.
See this repro:
```
evantest=# create subscription sub connection 'dbname=postgres' publication p
with (connect=false, max_retention_duration=-100);
WARNING: subscription was created, but is not connected
HINT: To initiate replication, you must manually create the replication slot,
enable the subscription, and alter the subscription to refresh publications.
CREATE SUBSCRIPTION
evantest=# select subname, submaxretention from pg_subscription where
subname='sub';
subname | submaxretention
---------+-----------------
sub | -100
(1 row)
```
I gave it -100, and it was stored in pg_subscription.
I went through the docs and didn’t find anything mentioning whether a negative
value is acceptable. But from the code, I think this indicates that a negative
value is meaningless:
```
/*
* Ensure that system configuration parameters are set appropriately to
* support retain_dead_tuples and max_retention_duration.
*/
CheckSubDeadTupleRetention(true, !opts.enabled, WARNING,
opts.retaindeadtuples, opts.retaindeadtuples,
(opts.maxretention > 0));
```
The last parameter of CheckSubDeadTupleRetention checks (opts.maxretention >
0), which implies that max_retention_duration is only meaningful when it is
greater than 0. Also, the docs say that 0 means “unlimited”.
From the following function, I think a negative value actually hurts:
```
/*
* Check whether conflict information retention should be stopped due to
* exceeding the maximum wait time (max_retention_duration).
*
* If retention should be stopped, return true. Otherwise, return false.
*/
static bool
should_stop_conflict_info_retention(RetainDeadTuplesData *rdt_data)
{
TimestampTz now;
Assert(TransactionIdIsValid(rdt_data->candidate_xid));
Assert(rdt_data->phase == RDT_WAIT_FOR_PUBLISHER_STATUS ||
rdt_data->phase == RDT_WAIT_FOR_LOCAL_FLUSH);
if (!MySubscription->maxretention)
return false;
/*
* Use last_recv_time when applying changes in the loop to avoid
* unnecessary system time retrieval. If last_recv_time is not available,
* obtain the current timestamp.
*/
now = rdt_data->last_recv_time ? rdt_data->last_recv_time :
GetCurrentTimestamp();
/*
* Return early if the wait time has not exceeded the configured maximum
* (max_retention_duration). Time spent waiting for table synchronization
* is excluded from this calculation, as it occurs infrequently.
*/
if (!TimestampDifferenceExceeds(rdt_data->candidate_xid_time, now,
MySubscription->maxretention +
rdt_data->table_sync_wait_time))
return false;
return true;
}
```
It only checks !MySubscription->maxretention, so a negative value will pass.
Then, in TimestampDifferenceExceeds, the negative value is counted, causing the
actual wait time to be shorter than the intended value.
So, I think we should reject negative values in the first place. The attached
tiny patch rejects negative max_retention_duration.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
v1-0001-Reject-negative-max_retention_duration-values.patch
Description: Binary data
