Re: [HACKERS] Restrict concurrent update/delete with UPDATE of partition key

On Wed, Mar 28, 2018 at 2:12 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> How will it break it? They'll see an invalid ctid and conclude that the
> tuple is dead? Without any changes that's already something that can
> happen if a later tuple in the chain has been pruned away. Sure, that
> code won't realize it should error out because the tuple is now in a
> different partition, but neither would a infomask bit.
>
> I think my big problem is that I just don't see what the worst that can
> happen is. We'd potentially see a broken ctid chain, something that very
> commonly happens, and consider the tuple to be invisible. That seems
> pretty sane behaviour for unadapted code, and not any worse than other
> potential solutions.

This is more or less my feeling as well. I think it's better to
conserve our limited supply of infomask bits as much as we can, and I
do think that we should try to reclaimed HEAP_MOVED_IN and
HEAP_MOVED_OFF in the future instead of defining the combination of
the two of them to mean something now.

The only scenario in which I can see this patch really leading to
disaster is if there's some previous release out there where the bit
pattern chosen by this patch has some other, incompatible meaning. As
far as we know, that's not the case: this bit pattern was previously
unused. Code seeing that bit pattern could potentially therefore (1)
barf on the valid CTID, but the whole point of this is to throw an
ERROR anyway, so if that happens then we're getting basically the
right behavior with the wrong error message or (2) just treat it as a
broken CTID link, in which case the result should be pretty much the
same as if this patch hadn't been committed in the first place.

Where the multixact patch really caused us a lot of trouble is that
the implications weren't just for the heap itself -- the relevant
SLRUs became subject to new retention requirements which in turn
affected vacuum, autovacuum, and checkpoint behavior. There is no
similar problem here -- the flag indicating the problematic situation,
however it ends up being stored, doesn't point to any external data.
Now, that doesn't mean there can't be some other kind of problem with
this patch, but I don't think that we should block the patch on the
theory that it might have an undiscovered problem that destroys the
entire PostgreSQL ecosystem with no theory as to what that problem
might actually be. Modulo implementation quality, I think the risk
level of this patch is somewhat but not vastly higher than
37484ad2aacef5ec794f4dd3d5cf814475180a78, which similarly defined a
previously-unused bit pattern in the tuple header. The reason I think
this one might be somewhat riskier is because AFAICS it's not so easy
to make sure we've found all the code, even in core, that might care,
as it was in that case; and also because updates happen more than
freezing.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company