On Tue, Feb 23, 2021 at 9:15 AM Álvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > On 2021-Feb-22, Álvaro Herrera wrote: > > > Here's an updated patch. > > > > I haven't added commentary or documentation to account for the new > > assumption, per Matthias' comment and Robert's discussion thereof. > > Done that. I also restructured the code a bit, since one line in > ComputedXidHorizons was duplicative.
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. FWIW, just for the record, around the following original code, /* * Check whether there are replication slots requiring an older xmin. */ h->shared_oldest_nonremovable = TransactionIdOlder(h->shared_oldest_nonremovable, h->slot_xmin); h->data_oldest_nonremovable = TransactionIdOlder(h->data_oldest_nonremovable, h->slot_xmin); /* * The only difference between catalog / data horizons is that the slot's * catalog xmin is applied to the catalog one (so catalogs can be accessed * for logical decoding). Initialize with data horizon, and then back up * further if necessary. Have to back up the shared horizon as well, since * that also can contain catalogs. */ h->shared_oldest_nonremovable_raw = h->shared_oldest_nonremovable; 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); 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. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/