Re: posgres 12 bug (partitioned table)

Hi,

On 2021-04-21 15:10:09 -0400, Tom Lane wrote:
> Indeed, it seems like this thread is putting a fair amount of work
> into a goal that we shouldn't even be trying to do.  We gave up the
> assumption that a partitioned table's children would have consistent
> system columns the moment we decided to allow foreign tables as
> partition leaf tables; and it's only going to get worse as alternate
> table AMs become more of a thing.

Agreed.


> So now I'm thinking the right thing to be working on is banning access
> to system columns via partition parent tables (other than tableoid,
> which ought to work).

And ctid, I assume? While I have some hope for generalizing the
representation of tids at some point, I don't think it's realistic that
we'd actually get rid of them anytime soon.


> I'm not quite sure whether the right way to do that is just not
> make pg_attribute entries for system columns of partitioned tables,
> as we do for views; or make them but have a code-driven error if
> you try to use one in a query.  The second way is uglier, but
> it should allow a more on-point error message.

I'm leaning towards the approach of not having the pg_attribute entries
- it seems not great to have pg_attribute entries for columns that
actually cannot be queried. I don't think we can expect tooling querying
the catalogs to understand that.


> OTOH, if we start to have different sets of system columns for
> different table AMs, we can't have partitioned tables covering all of
> those sets.

One could even imagine partition root specific system columns...

If we wanted AMs to actually be able to do introduce their own set of
system columns we'd need to change their representation to some
degree.

ISTM that the minimal changes needed would be to reorder sysattr.h to
have TableOidAttributeNumber be -2 (and keep
SelfItemPointerAttributeNumber as -1). And then slowly change all code
to not reference FirstLowInvalidHeapAttributeNumber - which seems like a
*substantial* amount of effort, due to all the shifting of AttrNumber by
FirstLowInvalidHeapAttributeNumber to be able to represent system
columns in bitmaps.

But perhaps that's the wrong direction, and we instead should work
towards *removing* system columns besides tableoid and ctid? At least
the way they work in heapam doesn't really make xmin,xmax,cmin,cmax
particularly useful.  Wild speculation: Perhaps we ought to have some
parse-analysis-time handling for AM specific functions that return
additional information about a row, and that are evaluated directly
above (or even in) tableams?


> In the meantime, if the back branches fail with something like
> "virtual tuple table slot does not have system attributes" when
> trying to do this, that's not great but I'm not sure we should
> be putting effort into improving it.

Seems ok enough.

Greetings,

Andres Freund