Re: {CREATE INDEX, REINDEX} CONCURRENTLY improvements

On Tue, Feb 23, 2021 at 9:15 AM Álvaro Herrera <alvherre@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/