From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Robert Haas <robertmhaas@gmail.com>,
        Pavan Deolasee <pavan.deolasee@gmail.com>,
        amul sul <sulamul@gmail.com>, Amit Kapila <amit.kapila16@gmail.com>,
        Stephen Frost <sfrost@snowman.net>,
        PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: [HACKERS] Restrict concurrent update/delete with UPDATE of
 partition key
In-reply-to: <20180328163541.l5ksnqmpn6bl4rg2@alap3.anarazel.de>
References: 
 <CAAJ_b97w_GGV-k4ErxWTpz5sAgFJ4aUYMx0khfySvANmWRzsag@mail.gmail.com>
 <CAA4eK1Kn7EgUdnFnp0d+Jk7Chf88qsyzNBDfJ8OJsMGpKuX4mA@mail.gmail.com>
 <CAA4eK1KtsTqsGDggDCrz2O9Jgo7ma-Co-B!8+v3L2zWMA2NHm6A@mail.gmail.com>
 <CAAJ_b95GpE7Qh+17zY-d5p5xPfqcTSY5ppEiYyeGTmoPaw=zXQ@mail.gmail.com>
 <CABOikdPXwqkLGgTZZm2qYwTn4L69V36rCh55fFma1fAYbon7Vg@mail.gmail.com>
 <10896.1520521635@sss.pgh.pa.us>
 <CA+Tgmob5gn1oKCjBUu8t7Go4h_AZfN5yy22N_Jv0OK8advJoWA@mail.gmail.com>
 <8293.1520528639@sss.pgh.pa.us>
 <CA+TgmoZzaH3WMhyLg_STX6=J+5iqQ0GvRWBnQihgDarF_mWSYg@mail.gmail.com>
 <21103.1520534813@sss.pgh.pa.us>
 <20180328163541.l5ksnqmpn6bl4rg2@alap3.anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Wed, 28 Mar 2018 09:35:41 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1897.1522259544.1@sss.pgh.pa.us>
Date: Wed, 28 Mar 2018 13:52:24 -0400
Message-ID: <1898.1522259544@sss.pgh.pa.us>
Precedence: bulk

Andres Freund <andres@anarazel.de> writes:
> On 2018-03-08 13:46:53 -0500, Tom Lane wrote:
>> ... Breaking fundamental invariants like
>> "ctid points to this tuple or its update successor" is going to cause
>> trouble.  There's a lot of code that knows that; more than knows the
>> details of what's in xmax, I believe.

> Given, as explained nearby, we already do store transient data in the
> ctid for speculative insertions (i.e. ON CONFLICT), and it hasn't caused
> even a whiff of trouble, I'm currently not inclined to see a huge issue
> here.  It'd be great if you could expand on your concerns here a bit, we
> gotta figure out a way forward.

Just what I said.  There's a lot of code that knows how to follow tuple
update chains, probably not all of it in core, and this will break it.
But only in seldom-exercised corner cases, which is the worst of all
possible worlds from a reliability standpoint.  (I don't think ON CONFLICT
is a counterexample because, IIUC, it's not a persistent state.)

Given that there are other ways we could attack it, I think throwing away
this particular invariant is an unnecessarily risky solution.

>> I would've been happier about expending an infomask bit towards this
>> purpose.  Just eyeing what we've got, I can't help noticing that
>> HEAP_MOVED_OFF/HEAP_MOVED_IN couldn't possibly be set in any tuple
>> in a partitioned table.  Perhaps making these tests depend on
>> partitioned-ness would be unworkably messy, but it's worth thinking
>> about.

> They previously couldn't be set together IIRC, so we could just (mask &
> (HEAP_MOVED_OFF |HEAP_MOVED_IN)) == (HEAP_MOVED_OFF |HEAP_MOVED_IN) but
> that'd be permanently eating two infomask bits.

Hmm.  That objection only matters if we have realistic intentions of
reclaiming those bits in future, which I've not heard anyone making
serious effort towards.  Rather than messing with the definition of ctid,
I'd be happier with saying that they're never going to be reclaimed, but
at least we're getting one bit's worth of real use out of them.

			regards, tom lane