Re: Do we need to handle orphaned prepared transactions in the server?

Thank you so much Bruce and Sawada.

Bruce:
I'll update the documentation with more details for max_age_prepared_xacts

Sawada:
You have raised 4 very valid points. Here are my thoughts.

(1)
I think your concern is that if we can reduce the need of 2 GUCs to one.

The purpose of having 2 different GUCs was serving two different purposes.
- max_age_prepared_xacts; this defines the maximum age of a prepared
transaction after which it may be considered an orphan.
- prepared_xacts_vacuum_warn_timeout; since we are throwing warnings in the
log, we need a way of controlling the behaviour to prevent from flooding
the log file with our messages. This timeout defines that. May be there is
a better way of handling this, but may be not.

(2)
Your point is that when there are more than one prepared transactions (and
even if the first one is removed), timeout
prepared_xacts_vacuum_warn_timeout isn't always accurate.

Yes, I agree. However, for us to hit the exact timeout for each prepared
transaction, we need setup timers and callback functions. That's too much
of an overhead IMHO. The alternative design that I took (the current
design) is based on the assumption that we don't need a precise timeout for
these transactions or for vacuum to report these issues to log. So, a
decent enough way of setting a timeout should be good enough considering
that it doesn't add any real overhead to the vacuum process. This does mean
that an orphaned prepared transaction may be notified after
prepared_xacts_vacuum_warn_timeout * 2. This, IMHO should be acceptable
behavior.

(3)
Message is too detailed.

Yes, I agree. I'll review this an update the patch.

(4)
GUCs should be renamed.

Yes, I agree. The names you have suggested make more sense. I'll send an
updated version of the patch with the new names and other suggested changes.

On Wed, Mar 11, 2020 at 10:43 AM Masahiko Sawada <
masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:

> On Mon, 2 Mar 2020 at 21:42, Hamid Akhtar <hamid(dot)akhtar(at)gmail(dot)com> wrote:
> >
> > Here is the v2 of the same patch after rebasing it and running it
> through pgindent. There are no other code changes.
> >
>
> Thank you for working on this. I think what this patch tries to
> achieve would be helpful to inform orphaned prepared transactions that
> can be cause of inefficient vacuum to users.
>
> As far as I read the patch, the setting this feature using newly added
> parameters seems to be complicated to me. IIUC, even if a prepared
> transactions is enough older than max_age_prepared_xacts, we don't
> warn if it doesn't elapsed prepared_xacts_vacuum_warn_timeout since
> when the "first" prepared transaction is created. And the first
> prepared transaction means that the first entry for
> TwoPhaseStateData->prepXacts. Therefore, if there is always more than
> one prepared transaction, we don't warn for at least
> prepared_xacts_vacuum_warn_timeout seconds even if the first added
> prepared transaction is already removed. So I'm not sure how we can
> think the setting value of prepared_xacts_vacuum_warn_timeout.
>
> Regarding the warning message, I wonder if the current message is too
> detailed. If we want to inform that there is orphaned prepared
> transactions to users, it seems to me that it's enough to report the
> existence (and possibly the number of orphaned prepared transactions),
> rather than individual details.
>
> Given that the above things, we can simply think this feature; we can
> have only max_age_prepared_xacts, and autovacuum checks the minimum of
> prepared_at of prepared transactions, and compare it to
> max_age_prepared_xacts. We can warn if (CurrentTimestamp -
> min(prepared_at)) > max_age_prepared_xacts. In addition, if we also
> want to control this behavior by the age of xid, we can have another
> GUC parameter for comparing the age(min(xid of prepared transactions))
> to that value.
>
> Finally, regarding the name of parameters, when we mention the age of
> transaction it means the age of xid of the transaction, not the time.
> Please refer to other GUC parameter having "age" in its name such as
> autovacuum_freeze_max_age, vacuum_freeze_min_age. The patch adds
> max_age_prepared_xacts but I think it should be renamed. For example,
> prepared_xact_warn_min_duration is for time and
> prepared_xact_warn_max_age for age.
>
> Regards,
>
> --
> Masahiko Sawada http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

--
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950 EMAIL: mailto:hamid(dot)akhtar(at)highgo(dot)ca
SKYPE: engineeredvirus