On Tue, 4 Apr 2023 at 17:08, Aleksander Alekseev
<aleksan...@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 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.
Alexander,
Ok, nice! I think it could be moved to committer then.
Pavel.
