From: Tom Lane <tgl@sss.pgh.pa.us>
To: Michael Paquier <michael@paquier.xyz>
cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org,
        Dilip Kumar <dilipbalaut@gmail.com>,
        Robert Haas <robertmhaas@gmail.com>
Subject: Re: Move pg_attribute.attcompression to earlier in struct for reduced
 size?
In-reply-to: <3962395.1621609263@sss.pgh.pa.us>
References: <20210517204803.iyk5wwvwgtjcmc5w@alap3.anarazel.de>
 <2854987.1621285592@sss.pgh.pa.us>
 <20210517212857.qyk4fxykfbxhmxof@alap3.anarazel.de>
 <YKMXVHCoNGZ61E9q@paquier.xyz> <YKdT5ZM5OInCsDkM@paquier.xyz>
 <3962395.1621609263@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Fri, 21 May 2021 11:01:03 -0400"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <626612.1621787110.1@sss.pgh.pa.us>
Date: Sun, 23 May 2021 12:25:10 -0400
Message-ID: <626613.1621787110@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/626613.1621787110%40sss.pgh.pa.us>
Precedence: bulk

I wrote:
> I think we need to do more than that.  It's certainly not okay to
> leave catalogs.sgml out of sync with reality.  And maybe I'm just
> an overly anal-retentive sort, but I think that code that manipulates
> tuples ought to match the declared field order if there's not some
> specific reason to do otherwise.  So that led me to the attached.

Pushed that after another round of review.

> ... there are two places that set
> up attcompression depending on
>     if (rel->rd_rel->relkind == RELKIND_RELATION ||
>         rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> This seems fairly nuts; in particular, why are matviews excluded?

While I've not actually tested this, it seems to me that we could
just drop these relkind tests altogether.  It won't hurt anything
to set up attcompression in relation descriptors where it'll never
be consulted.

However, the more I looked at that code the less I liked it.
I think the way that compression selection is handled for indexes,
ie consult default_toast_compression on-the-fly, is *far* saner
than what is currently implemented for tables.  So I think we
should redefine attcompression as "ID of a compression method
to use, or \0 to select the prevailing default.  Ignored if
attstorage does not permit the use of compression".  This would
result in approximately 99.44% of all columns just having zero
attcompression, greatly simplifying the tupdesc setup code, and
also making it much easier to flip an installation over to a
different preferred compression method.

I'm happy to prepare a patch if that sketch sounds sane.

(Note that the existing comment claiming that attcompression
"Must be InvalidCompressionMethod if and only if typstorage is
'plain' or 'external'" is a flat out lie in any case; *both*
directions of that claim are wrong.)

			regards, tom lane