| From: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
|---|---|
| To: | PostgreSQL-development <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Cc: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com> |
| Subject: | Reject negative max_retention_duration values |
| Date: | 2026-06-09 08:05:23 |
| Message-ID: | 9232401A-DEEE-49E1-9D11-D14A776DB82B@gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
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/
| Attachment | Content-Type | Size |
|---|---|---|
| v1-0001-Reject-negative-max_retention_duration-values.patch | application/octet-stream | 5.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | jian he | 2026-06-09 08:10:45 | Re: Move FOR PORTION OF checks out of analysis |
| Previous Message | Kyotaro Horiguchi | 2026-06-09 07:59:38 | Re: Why is the LSN reported for pg_logical_emit_message() different from other decoded operations? |