From: Antonin Houska <ah@cybertec.at>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: Marcos Pegoraro <marcos@f10.com.br>, Michael Banck <mbanck@gmx.net>,
    Junwang Zhao <zhjwpku@gmail.com>,
    Kirill Reshke <reshkekirill@gmail.com>,
    Pavel Stehule <pavel.stehule@gmail.com>,
    Michael Paquier <michael@paquier.xyz>,
    PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: why there is not VACUUM FULL CONCURRENTLY?
In-reply-to: <202503251432.o6obtgywjow5@alvherre.pgsql>
References: <202503251432.o6obtgywjow5@alvherre.pgsql>
Comments: In-reply-to Alvaro Herrera <alvherre@alvh.no-ip.org>
   message dated "Tue, 25 Mar 2025 15:32:46 +0100."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <80296.1742989179.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 26 Mar 2025 12:39:39 +0100
Message-ID: <80297.1742989179@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/80297.1742989179%40localhost>
Precedence: bulk

Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> On 2025-Mar-22, Antonin Houska wrote:
> =

> > Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > =

> > > I rebased this patch series; here's v09.  No substantive changes fro=
m 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 nee=
d that
> > > change in the first place.  According to the comment where the progr=
ess
> > > restore function is called, it's because reorderbuffer.c uses a
> > > subtransaction internally.  But I went to look at reorderbuffer.c an=
d
> > > 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 ha=
ving
> > > to open a transaction block.
> > =

> > Which part of reorderbuffer.c do you mean? ISTM that the use of subran=
sactions
> > is more extensive. At least ReorderBufferImmediateInvalidation() appea=
rs 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-lev=
el
transaction. However REPACK CONCURRENTLY should always run in a transactio=
n,
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=3D0x5606dc2a8c88, keyPtr=3D=
0x7ffeab341928, action=3DHASH_FIND, foundPtr=3D0x0)
>     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=3D0x5606dc2a8c88, keyPtr=3D=
0x7ffeab341928, action=3DHASH_FIND, foundPtr=3D0x0)
>     at ../../../../../../../../../pgsql/source/master/src/backend/utils/=
hash/dynahash.c:960
> #1  0x000055d4fbea0a46 in is_concurrent_repack_in_progress (relid=3D2197=
3)
>     at ../../../../../../../../pgsql/source/master/src/backend/commands/=
cluster.c:2729
> #2  is_concurrent_repack_in_progress (relid=3Drelid@entry=3D2964)
>     at ../../../../../../../../pgsql/source/master/src/backend/commands/=
cluster.c:2706
> #3  0x000055d4fc237a87 in RelationBuildDesc (targetRelId=3D2964, insertI=
t=3DinsertIt@entry=3Dtrue)
>     at ../../../../../../../../../pgsql/source/master/src/backend/utils/=
cache/relcache.c:1257
> #4  0x000055d4fc239456 in RelationIdGetRelation (relationId=3D<optimized=
 out>, relationId@entry=3D2964)
>     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