On Wed, Mar 18, 2020 at 09:56:40AM +0100, Julien Rouhaud wrote:
> On Wed, Mar 18, 2020 at 04:55:25PM +0900, Michael Paquier wrote:
> >
> > It would be good to be careful about the indentation. Certain parts
> > of 0003 don't respect the core indentation. Not necessarily your job
> > though. Other than that 0003 seems to be in good shape.
>
> I'll try to do a partial pgindent run on all patches before next patchset.
I run a pgindent on all .[ch] files and kept only the relevant changes, for
each patch, so this should now be fine.
> >
> > @@ -54,6 +55,7 @@ recordDependencyOn(const ObjectAddress *depender,
> > void
> > recordMultipleDependencies(const ObjectAddress *depender,
> > const ObjectAddress *referenced,
> > + const char *version,
> > int nreferenced,
> > DependencyType behavior)
> > Nit from patch 0002: the argument "version" should be fourth I think,
> > keeping the number of referenced objects and the referenced objects
> > close. And actually, this "version" argument is removed in patch
> > 0004, replaced by the boolean track_version. (By reading the
> > arguments below I'd rather keep *version).
I changed 0002 to have the version as the 4th argument just in case.
> > So, 0004 is the core of the changes. I have found a bug with the
> > handling of refobjversion and pg_depend entries. When swapping the
> > dependencies of the old and new indexes in index_concurrently_swap(),
> > refobjversion remains set to the value of the old index. I used a
> > manual UPDATE on pg_depend to emulate that with a past fake version
> > string to emulate that (sneaky I know), but you would get the same
> > behavior with an upgraded instance. refobjversion should be updated
> > to the version of the new index.
>
> Oh good catch. I'll dig into it.
AFAICT it was only missing a call to index_update_collation_versions() in
ReindexRelationConcurrently. I added regression tests to make sure that
REINDEX, REINDEX [INDEX|TABLE] CONCURRENTLY and VACUUM FULL are doing what's
expected.
Given discussion in nearby threads, I obviously can't add tests for failed
REINDEX CONCURRENTLY, so here's what's happening with a manual repro:
=# CREATE TABLE t1(id integer, val text);
CREATE
=# CREATE INDEX ON t1(val COLLATE "fr-x-icu");
CREATE
=# UPDATE pg_depend SET refobjversion = 'meh' WHERE refobjversion = '153.97';
UPDATE 1
=# REINDEX TABLE CONCURRENTLY t1 ;
LOCATION: ReindexRelationConcurrently, indexcmds.c:2839
^CCancel request sent
ERROR: 57014: canceling statement due to user request
LOCATION: ProcessInterrupts, postgres.c:3171
=# SELECT objid::regclass, indisvalid, refobjversion
FROM pg_depend d
JOIN pg_index i ON i.indexrelid = d.objid
WHERE refobjversion IS NOT NULL;
objid | indisvalid | refobjversion
------------------+------------+---------------
t1_val_idx_ccold | f | 153.97
t1_val_idx | t | meh
(2 rows)
=# REINDEX TABLE t1;
WARNING: 0A000: cannot reindex invalid index "pg_toast.pg_toast_16418_index_ccold" on TOAST table, skipping
LOCATION: reindex_relation, index.c:3987
REINDEX
=# SELECT objid::regclass, indisvalid, refobjversion
FROM pg_depend d
JOIN pg_index i ON i.indexrelid = d.objid
WHERE refobjversion IS NOT NULL;
objid | indisvalid | refobjversion
------------------+------------+---------------
t1_val_idx_ccold | t | 153.97
t1_val_idx | t | 153.97
(2 rows)
ISTM that it's working as intended.
> > + if (track_version)
> > + {
> > + /* Only dependency on a collation is supported. */
> > + if (referenced->classId == CollationRelationId)
> > + {
> > + /* C and POSIX collations don't require tracking the version */
> > + if (referenced->objectId == C_COLLATION_OID ||
> > + referenced->objectId == POSIX_COLLATION_OID)
> > + continue;
> > I don't think that the API is right for this stuff, as you introduce
> > collation-level knowledge into something which has been much more
> > flexible until now. Wouldn't it be better to move the refobjversion
> > string directly into ObjectAddress?
>
> We could, but we would then need to add code to retrieve the collation version
> in multiple places (at least RecordDependencyOnCollation and
> recordDependencyOnSingleRelExpr). I'm afraid that'll open room for bugs if
> some other places are missed, now or later, even more if more objects get a
> versionning support.
No change here.
> > + /*
> > + * Perform version sanity checks on the relation underlying indexes if
> > + * it's not a VACUUM FULL
> > + */
> > + if (!(options & VACOPT_FULL) && onerel && !IsSystemRelation(onerel) &&
> > + onerel->rd_rel->relhasindex)
> > Should this explain why?
Explanation added.
v17 attached, rebased against master (conflict since 8408e3a557).