Date: Fri, 30 Aug 2024 16:06:03 -0500
From: Nathan Bossart <nathandbossart@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Ayush Vatsa <ayushvatsa1810@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	michael@paquier.xyz
Subject: Re: Pgstattuple on Sequences: Seeking Community Feedback on
 Potential Patch
Message-ID: <ZtI0O-bN90P02oQO@nathan>
References: 
 <CACX+KaP3i+i9tdPLjF5JCHVv93xobEdcd_eB+638VDvZ3i=cQA@mail.gmail.com>
 <Zsy6w_yW2QKYSo_m@nathan>
 <CA+TgmoYm0X=KuHry1A6prNzf3pUmAcXYgQEr25kCY8jSMJaVjg@mail.gmail.com>
 <ZszGBMK1D4hCsRnD@nathan>
 <CACX+KaNhg-ZwkqCSjsq7j_gRYDmHip2b4kwa6ohac0MspSeW9w@mail.gmail.com>
 <ZtCxo95K_bqGbmNQ@nathan>
 <CA+TgmoaHX9BYrr1u9c356ovGwVBy25Q9y_LdSYS8JW0pqd18JA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CA+TgmoaHX9BYrr1u9c356ovGwVBy25Q9y_LdSYS8JW0pqd18JA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZtI0O-bN90P02oQO%40nathan>
Precedence: bulk

On Fri, Aug 30, 2024 at 04:07:30PM -0400, Robert Haas wrote:
> On Thu, Aug 29, 2024 at 1:36 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Thanks.  Robert, do you have any concerns with this?
> 
> I don't know if I'm exactly concerned but I don't understand what
> problem we're solving, either. I thought Ayush said that the function
> wouldn't produce useful results for sequences; so then why do we need
> to change the code to enable it?

I suppose it would be difficult to argue that it is actually useful, given
it hasn't worked since v11 and apparently nobody noticed until recently.
If we're content to leave it unsupported, then sure, let's just remove the
"relkind == RELKIND_SEQUENCE" check in pgstat_relation().  But I also don't
have a great reason to _not_ support it.  It used to work (which appears to
have been intentional, based on the code), it was unintentionally broken,
and it'd work again with a ~1 line change.  "SELECT count(*) FROM
my_sequence" probably doesn't provide a lot of value, but I have no
intention of proposing a patch that removes support for that.

All that being said, I don't have a terribly strong opinion, but I guess I
lean towards re-enabling.

Another related inconsistency I just noticed in pageinspect:

    postgres=# select t_data from heap_page_items(get_raw_page('s', 0));
                    t_data
    --------------------------------------
     \x0100000000000000000000000000000000
    (1 row)

    postgres=# select tuple_data_split('s'::regclass, t_data, t_infomask, t_infomask2, t_bits) from heap_page_items(get_raw_page('s', 0));
    ERROR:  only heap AM is supported

-- 
nathan