Re: could not read block 0 in file : read only 0 of 8192 bytes when doing nasty on immutable index function

On 2018-07-31 18:48:23 -0700, Peter Geoghegan wrote:
> On Mon, Jul 9, 2018 at 11:32 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > I assume we'll have to backpatch this issue, so I think it'd probably a
> > good idea to put a specific CacheInvalidateHeapTuple() in there
> > explicitly in the back branches, and do the larger fix in 12. ISTM
> > there's some risks that it'd cause issues.
>
> What do you think of the attached?
>
> The is a new CacheInvalidateRelcache() call, rather than a new call to
> CacheInvalidateRelcacheByTuple(), but those two things are equivalent
> (I assume that you actually meant to say
> CacheInvalidateRelcacheByTuple(), not CacheInvalidateHeapTuple()).

Right.

> From 18ffbcc81c75525c73930ad3b5a63ae0873d2381 Mon Sep 17 00:00:00 2001
> From: Peter Geoghegan <pg(at)bowt(dot)ie>
> Date: Tue, 31 Jul 2018 18:33:30 -0700
> Subject: [PATCH] Add table relcache invalidation to index builds.
>
> It's necessary to make sure that owning tables have a relcache
> invalidation prior to advancing the command counter to make
> newly-entered catalog tuples for the index visible. inval.c must be
> able to roll back the local caches in the event of transaction abort.
> There is only a problem when CREATE INDEX transactions abort, since
> there is a generic invalidation once we reach index_update_stats().
>
> This bug is of long standing. Problems were made much more likely by
> the addition of parallel CREATE INDEX (commit 9da0cc35284), but it is
> strongly suspected that similar problems can be triggered without
> involving plan_create_index_workers().

Maybe expand a bit on this by saying that it's more likely "because
plan_create_index_workers() triggers a relcache entry to be (re-)built,
which previously did only happen in edge cases" or such?

> Bug diagnosed by Andres Freund.
>
> Author: Peter Geoghegan
> Reported-By: Luca Ferrari
> Discussion: https://postgr.es/m/CAKoxK+5fVodiCtMsXKV_1YAKXbzwSfp7DgDqUmcUAzeAhf=HEQ@mail.gmail.com
> Backpatch: 9.3-
> ---
> src/backend/catalog/index.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
> index 8b276bc430..7036d72bd6 100644
> --- a/src/backend/catalog/index.c
> +++ b/src/backend/catalog/index.c
> @@ -1137,6 +1137,19 @@ index_create(Relation heapRelation,
> InvokeObjectPostCreateHookArg(RelationRelationId,
> indexRelationId, 0, is_internal);
>
> + /*
> + * Invalidate the relcache for the owning table, so that any local
> + * relcache entry for the new index built after CommandCounterIncrement
> + * won't become inconsistent in the event of transaction abort. inval.c
> + * must be able to roll back the local caches when aborting. Clearly it
> + * isn't useful to create an index whose definition results in a relcache
> + * entry for the index being allocated before commit, but the local caches
> + * cannot be allowed to become inconsistent under any circumstances.
> + * (There is only an issue when transactions abort because we'll reach
> + * index_update_stats when they commit.)
> + */

Not a fan of this comment. It doesn't really explain that well why it's
needed here, but then goes on to a relatively general explanation of why
cache invalidation is necessary. Why not just go for something like
"register relcache invalidation on the indexes' heap relation, to
maintain consistency of its index list"?

I wonder if it wouldn't be more appropriately placed closer to the
UpdateIndexRelation(), given that that's essentially what necessitates
the relcache flush?

Greetings,

Andres Freund