agora inbox for [email protected]
help / color / mirror / Atom feedFrom: Antonin Houska <[email protected]>
To: Alvaro Herrera <[email protected]>
Cc: Marcos Pegoraro <[email protected]>
Cc: Michael Banck <[email protected]>
Cc: Junwang Zhao <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: Pavel Stehule <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: why there is not VACUUM FULL CONCURRENTLY?
Date: Wed, 26 Mar 2025 12:39:39 +0100
Message-ID: <80297.1742989179@localhost> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
Alvaro Herrera <[email protected]> wrote:
> On 2025-Mar-22, Antonin Houska wrote:
>
> > Alvaro Herrera <[email protected]> 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@entry=2964)
> at ../../../../../../../../pgsql/source/master/src/backend/commands/cluster.c:2706
> #3 0x000055d4fc237a87 in RelationBuildDesc (targetRelId=2964, insertIt=insertIt@entry=true)
> at ../../../../../../../../../pgsql/source/master/src/backend/utils/cache/relcache.c:1257
> #4 0x000055d4fc239456 in RelationIdGetRelation (relationId=<optimized out>, relationId@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
view thread (89+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: why there is not VACUUM FULL CONCURRENTLY?
In-Reply-To: <80297.1742989179@localhost>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox