On Sat, Aug 16, 2025 at 09:22:18AM +0900, Michael Paquier wrote:
> On Fri, Aug 15, 2025 at 12:36:00PM +0500, Kirill Reshke wrote:
> > 0001 - LGTM?
>
> Thanks for the review. Yes, I am eyeing at applying this refactoring
> piece to limit the footprint of Form_pg_sequence_data. The
> expectations of reset_state are not completely documented at the top
> of init_params(), because it is not only about nextval(). I'll try to
> think about a more careful wording.
Applied this one after more tweaking.
>> 0002. WFM, the sole observation I have is that we incorporate sequence
>> logic to AlterTable utilities, making it less clear of what they
>> actually do.
>
> Thanks, I am also looking at applying this part of the patch set.
> There is a second reason for that: we don't track yet what kind of
> attributes are created within a CREATE SEQUENCE, so ADD COLUMN TO
> SEQUENCE is IMO also useful on its own to make user-facing some of the
> actions taken internally by the backend in this case. Perhaps there's
> an argument that people do not like the AddColumnToView layer, and
> that we should not expand that to sequences, but I like this new
> separation concept for the deparsing logistics.
For this one, I'm having some second thoughts after re-reading Tomas
point upthread, which is very close to your point. It is true that
using AlterColumn is an implementation detail of sequences, because we
store some of their data as heap. Using AddColumnToSequence feels
indeed a bit out of place after more consideration. We could just
reuse AlterColumn, saving from the extra sub-command. What do you
think?
Rebased the rest as attached, for now.
--
Michael
