Reject negative max_retention_duration values

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

Responses

Browse pgsql-hackers by date

  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?