On 2021-Feb-23, Masahiko Sawada wrote: > I've looked at the v3 patch and it looks good to me. A minor comment is: > > + /* Catalog tables need to consider all backends. */ > + h->catalog_oldest_nonremovable = > + TransactionIdOlder(h->catalog_oldest_nonremovable, xmin); > + > > I think that the above comment is not accurate since we consider only > backends on the same database in some cases.
You're right. Adjusted. > the patch makes the following change: > > @@ -1847,7 +1871,6 @@ ComputeXidHorizons(ComputeXidHorizonsResult *h) > h->shared_oldest_nonremovable = > TransactionIdOlder(h->shared_oldest_nonremovable, > h->slot_catalog_xmin); > - h->catalog_oldest_nonremovable = h->data_oldest_nonremovable; > h->catalog_oldest_nonremovable = > TransactionIdOlder(h->catalog_oldest_nonremovable, > h->slot_catalog_xmin); > > In the original code, catalog_oldest_nonremovable is initialized with > data_oldest_nonremovable that considered slot_xmin. Therefore, IIUC > catalog_oldest_nonremovable is the oldest transaction id among > data_oldest_nonremovable, slot_xmin, and slot_catalog_xmin. But with > the patch, catalog_oldest_nonremovable doesn't consider slot_xmin. It > seems okay to me as catalog_oldest_nonremovable is interested in only > slot_catalog_xmin. I tend to agree -- it seems safe to ignore slot_xmin in that case. However, tracing through the maze of xmin vs. catalog_xmin transmission across the whole system is not trivial. I changed the patch so that we adjust to slot_xmin first, then to slot_catalog_xmin. I *think* the end effect should be the same. But if we want to make that claim to save that one assignment, we can make it in a separate commit. However, given that this comparison and assignment is outside the locked area, I don't think it's worth bothering with. Thanks for the input! I have pushed this patch. -- Álvaro Herrera Valdivia, Chile "El sentido de las cosas no viene de las cosas, sino de las inteligencias que las aplican a sus problemas diarios en busca del progreso." (Ernesto Hernández-Novich)
