On Tue, 4 Apr 2023 at 17:08, Aleksander Alekseev
<aleksander@timescale.com> wrote:
>
> 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
indexesand truncation).
>
> My bad. Fixed.
>
> > I'm also having second thoughts about "Ensure that autovacuum is progressing" in the hint. That might be better in
thedocs, 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-runningsessions. 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.
Alexander,
Ok, nice! I think it could be moved to committer then.
Pavel.
