Re: why there is not VACUUM FULL CONCURRENTLY?

Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:

> On 2025-Mar-22, Antonin Houska wrote:
>
> > Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> >
> > > I rebased this patch series; here's v09. No substantive changes from v08.
> > > I made sure the tree still compiles after each commit.
>
> I rebased again, fixing a compiler warning reported by CI and applying
> pgindent to each individual patch. I'm slowly starting to become more
> familiar with the whole of this new code.

Thanks.

> > > I did look at 0002 again [...], and ended up wondering why do we need that
> > > change in the first place. According to the comment where the progress
> > > restore function is called, it's because reorderbuffer.c uses a
> > > subtransaction internally. But I went to look at reorderbuffer.c and
> > > noticed that the subtransaction is only used "when using the SQL
> > > function interface, because that creates a transaction already". So
> > > maybe we should look into making REPACK use reorderbuffer without having
> > > to open a transaction block.
> >
> > Which part of reorderbuffer.c do you mean? ISTM that the use of subransactions
> > is more extensive. At least ReorderBufferImmediateInvalidation() appears to
> > rely on it, which in turn is called by xact_decode().
>
> Ah, right, I was not looking hard enough. Something to keep in mind --
> though I'm still not convinced that it's best to achieve this by
> introducng a mechanism to restore progress state. Maybe allowing a
> transaction to abort without clobbering the progress state somehow (not
> trivial to implement at present though, because of layers of functions
> you need to traverse with such a flag; maybe have a global in xact.c
> that you set by calling a function? Not sure -- might be worse.)

The problem seems to be specific to the use of BeginInternalSubTransaction():
if it is not called at given code paths, it means that there is no top-level
transaction. However REPACK CONCURRENTLY should always run in a transaction,
so it should always run BeginInternalSubTransaction(). Thus I think we can use
this function to set the flag.

> I also noticed that CI is complaining of a problem in Windows, which is
> easily reproducible in non-Windows by defining EXEC_BACKEND. The
> backtrace is this:
>
> #0 0x000055d4fc24fe96 in hash_search (hashp=0x5606dc2a8c88, keyPtr=0x7ffeab341928, action=HASH_FIND, foundPtr=0x0)
> at ../../../../../../../../../pgsql/source/master/src/backend/utils/hash/dynahash.c:960
> 960 return hash_search_with_hash_value(hashp,
> (gdb) bt
> #0 0x000055d4fc24fe96 in hash_search (hashp=0x5606dc2a8c88, keyPtr=0x7ffeab341928, action=HASH_FIND, foundPtr=0x0)
> at ../../../../../../../../../pgsql/source/master/src/backend/utils/hash/dynahash.c:960
> #1 0x000055d4fbea0a46 in is_concurrent_repack_in_progress (relid=21973)
> at ../../../../../../../../pgsql/source/master/src/backend/commands/cluster.c:2729
> #2 is_concurrent_repack_in_progress (relid=relid(at)entry=2964)
> at ../../../../../../../../pgsql/source/master/src/backend/commands/cluster.c:2706
> #3 0x000055d4fc237a87 in RelationBuildDesc (targetRelId=2964, insertIt=insertIt(at)entry=true)
> at ../../../../../../../../../pgsql/source/master/src/backend/utils/cache/relcache.c:1257
> #4 0x000055d4fc239456 in RelationIdGetRelation (relationId=<optimized out>, relationId(at)entry=2964)
> at ../../../../../../../../../pgsql/source/master/src/backend/utils/cache/relcache.c:2105
>
>
> So apparently we're trying to dereference a hash table which isn't
> properly set up in the child process.

I'll fix that.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com