public inbox for [email protected]  

help / color / mirror / Atom feed

From: Álvaro Herrera <[email protected]>
To: jian he <[email protected]>
Cc: PostgreSQL Hackers <[email protected]>
Subject: Re: using index to speedup add not null constraints to a table
Date: Tue, 13 Jan 2026 18:30:27 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <CACJufxHaNDft6Qw-O+ZKdztOE6J3KKoTPP7y39nqst7BLYGKEQ@mail.gmail.com>

Hello

I rebased this patch to review it.  Overall, I think this is a great
idea.  Here are some comments on the patch, which I hope are not
anything major.

I'm not really sure that I agree with the way ATRewriteTable works now.
It seems that we scan the list of columns, and verify each one for nulls,
but abort midway if a column is generated.  Surely we should check for
generated columns first, to avoid wasting time verifying earlier
columns in case a later column is generated?

That said, we do check for notnull_virtual_attrs to be NIL.  It seems to
me that this implies that the checked columns are not generated.  In
other words, the test for whether columns are generated is unnecessary
and confusing, and in which case you don't have to change anything, just
remove the "if (attr->attgenerated == ATTRIBUTE_GENERATED_VIRTUAL)"
inner block.

However, if we do this, then I think computing notnull_attrs is
pointless.  So we should only change the order: do this new check first,
and if we find that any new not-null column is on a generated column,
then we compute both notnull_virtual_attrs and notnull_attrs.  No?


The separation of labor between index_check_notnull() and
index_check_notnull_internal() seems a bit pointless.  Why not have a
single routine that does both things?  Though, to be honest, it does
look like the former should live in tablecmds.c instead of
execIndexing.c.  (But if you do split things that way, then you need to
make index_check_notnull_internal extern).

The tests look a bit excessive.  Why do we need an isolation test for
this?  And it looks to me like the other test could be in
src/test/regress rather than be a TAP test.  After all, that's what you
have the ereport(DEBUG1) there, isn't it?

"veritify" doesn't exist.  The word is "verify".

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: using index to speedup add not null constraints to a table
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox