Re: [PATCH] Clarify the behavior of the system when approaching XID wraparound

Hi,

> The proposed changes are in patchset v5.

Pavel, John, thanks for your feedback.

> I've looked into the patches v4.
> For 0001:
> I think long "not accepting commands that generate" is equivalent to
> more concise "can't generate".

Frankly I don't think this is a good change for this particular CF
entry and it violates the consistency with multixact.c. Additionally
the new message is not accurate. The DBMS _can_ generate new XIDs,
quite a few of them actually. It merely refuses to do so.

> For all:
> In a errhint's list what _might_ be done I think AND is a little bit
> better that OR as the word _might_ means that each of the proposals in
> the list is a probable, not a sure one.

Well, that's debatable... IMO "or" makes a bit more sense since the
user knows better whether he/she needs to kill a long-running
transaction, or run VACUUM, or maybe do both. "And" would imply that
the user needs to do all of it, which is not necessarily true. Since
originally it was "or" I suggest we keep it as is for now.

All in all I would prefer keeping the focus on the particular problem
the patch tries to address.

> For 0003:
> I think double mentioning of Vacuum at each errhist i.e.: "Execute a
> database-wide VACUUM in that database" and "...or run a manual
> database-wide VACUUM." are redundant. The advice "Ensure that
> autovacuum is progressing,..." is also not needed after advice to
> "Execute a database-wide VACUUM in that database".
> [...]

> Okay, great. For v4-0003:
>
> Each hint mentions vacuum twice, because it's adding the vacuum message at the end, but not removing it from the beginning. The vacuum string should be on its own line, since we will have to modify that for back branches (skip indexes and truncation).

My bad. Fixed.

> I'm also having second thoughts about "Ensure that autovacuum is progressing" in the hint. That might be better in the docs, if we decide to go ahead with adding a specific checklist there.

OK, done.

> In vacuum.c:
>
> errhint("Close open transactions soon to avoid wraparound problems.\n"
> - "You might also need to commit or roll back old prepared transactions, or drop stale replication slots.")));
> + "You might also need to commit or roll back old prepared transactions, drop stale replication slots, or kill long-running sessions. Ensure that autovacuum is progressing, or run a manual database-wide VACUUM.")));
>
> This appears in vacuum_get_cutoffs(), which is called by vacuum and cluster, and the open transactions were already mentioned, so this is not the place for this change.

Fixed.

> v4-0002:
>
> - errhint("Stop the postmaster and vacuum that database in single-user mode.\n"
> + errhint("VACUUM that database.\n"
>
> Further in the spirit of consistency, the mulixact path already has "Execute a database-wide VACUUM in that database.\n", and that seems like better wording.

Agree. Fixed.

--
Best regards,
Aleksander Alekseev