On Mon, 2 Mar 2020 at 21:42, Hamid Akhtar <hamid.akh...@gmail.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
