pg.ddx.io  pgsql-hackers@postgresql.org mailing list archive  
help / color / mirror / Atom feed
From: Michael Paquier <michael@paquier.xyz>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jonathan S. Katz <jkatz@postgresql.org>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Large expressions in indexes can't be stored (non-TOASTable)
Date: Tue, 24 Sep 2024 13:21:45 +0900
Message-ID: <ZvI-WXcaFXuj1Dvp@paquier.xyz> (raw)
In-Reply-To: <ZvGOPdGAronxOfmf@nathan>
References: <c500a379-7a20-4ab4-bcc9-5ea9aabe017a@postgresql.org>
	<ZuroEfWjxd4Zj6Yq@nathan>
	<1812111.1726671296@sss.pgh.pa.us>
	<ZusvlZBrPKm913LA@nathan>
	<a97d7401-e7c9-f771-6a00-037379f0a8bb@gmail.com>
	<ZuxvNDVfqpahqZNb@nathan>
	<adb7421f-eaf4-878a-6742-576fc6bf0d8f@gmail.com>
	<Zu2oJtMI2avvjmtl@nathan>
	<e012fb69-ca34-7e0e-3a33-b16d9737e526@gmail.com>
	<ZvGOPdGAronxOfmf@nathan>

On Mon, Sep 23, 2024 at 10:50:21AM -0500, Nathan Bossart wrote:
> I carefully inspected all the code paths this patch touches, and I think
> I've got all the details right, but I would be grateful if someone else
> could take a look.

No objections from here with putting the snapshots pops and pushes
outside the inner routines of reindex/drop concurrently, meaning that
ReindexRelationConcurrently(), DefineIndex() and index_drop() are fine
to do these operations.

Looking at the patch, we could just add an assertion based on
ActiveSnapshotSet() in index_set_state_flags().

Actually, thinking more...  Could it be better to have some more
sanity checks in the stack outside the toast code for catalogs with
toast tables?  For example, I could imagine adding a check in
CatalogTupleUpdate() so as all catalog accessed that have a toast 
relation require an active snapshot.  That would make checks more
aggressive, because we would not need any toast data in a catalog to
make sure that there is a snapshot set.  This strikes me as something
we could do better to improve the detection of failures like the one
reported by Alexander when updating catalog tuples as this can be
triggered each time we do a CatalogTupleUpdate() when dirtying a
catalog tuple.  The idea is then to have something before the
HaveRegisteredOrActiveSnapshot() in the toast internals, for catalogs,
and we would not require toast data to detect problems.
--
Michael

Attachments:

  signature.asc (application/pgp-signature, 832B) download

view thread (62+ messages)  latest in thread

Message-ID: <ZvI-WXcaFXuj1Dvp@paquier.xyz>
Permalink:  ../ZvI-WXcaFXuj1Dvp@paquier.xyz/
Also on:    postgresql.org/message-id/ZvI-WXcaFXuj1Dvp@paquier.xyz

 · 

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: pgsql-hackers@postgresql.org
  Cc: michael@paquier.xyz, nathandbossart@gmail.com, exclusion@gmail.com, tgl@sss.pgh.pa.us, jkatz@postgresql.org, pgsql-hackers@lists.postgresql.org
  Subject: Re: Large expressions in indexes can't be stored (non-TOASTable)
  In-Reply-To: <ZvI-WXcaFXuj1Dvp@paquier.xyz>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox