Re: {CREATE INDEX, REINDEX} CONCURRENTLY improvements

On Tue, Feb 23, 2021 at 9:15 AM Álvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)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/