Thread: using index or check in ALTER TABLE SET NOT NULL
Hello I write patch to speed up ALTER TABLE SET NOT NULL by check existed check constraints or indexes. Huge phase 3 with verifytable data will be skipped if table has valid check constraint cover "alteredfield IS NOT NULL" condition or by SPIquery if found index with compatible condition or regular amsearchnulls index on processed field. Patch based on current master branch, i believe it has no platform-dependent code, of course code compiled and pass testslocally. Tell me please, what i forgot or make incorrectly. Implementation notes: I use existed PartConstraintImpliedByRelConstraint method to check relation constraints. But i rename original method tostatic ConstraintImpliedByRelConstraint (because method now used not only in partitions) and leave PartConstraintImpliedByRelConstraintas proxy to not change public API. I found it difficult to do index scan and choose index with lower costs if found many suitable indexes. Is it acceptableto use SPI here? Related archive discussions: https://www.postgresql.org/message-id/flat/530C10CF.4020101%40strangersgate.com https://www.postgresql.org/message-id/flat/CAASwCXdAK55BzuOy_FtYj2zQWg26PriDKL5pRoWiyFJe0eg-Hg%40mail.gmail.com Thanks!
Attachment
On Tue, Nov 28, 2017 at 1:59 PM, Sergei Kornilov <sk@zsrv.org> wrote: > I write patch to speed up ALTER TABLE SET NOT NULL by check existed check constraints or indexes. Huge phase 3 with verifytable data will be skipped if table has valid check constraint cover "alteredfield IS NOT NULL" condition or by SPIquery if found index with compatible condition or regular amsearchnulls index on processed field. Doing this based on the existence of a valid constraint which implies that no nulls can be present seems like a good idea. Doing it based on an index scan doesn't necessarily seem like a good idea. We have no guarantee at all that the index scan will be faster than scanning the table would have been, and a single table scan can do multiple verification steps if, for example, multiple columns are set NOT NULL at the same time. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Tue, Nov 28, 2017 at 1:59 PM, Sergei Kornilov <sk@zsrv.org> wrote: > > I write patch to speed up ALTER TABLE SET NOT NULL by check existed check constraints or indexes. Huge phase 3 with verifytable data will be skipped if table has valid check constraint cover "alteredfield IS NOT NULL" condition or by SPIquery if found index with compatible condition or regular amsearchnulls index on processed field. > > Doing this based on the existence of a valid constraint which implies > that no nulls can be present seems like a good idea. Doing it based > on an index scan doesn't necessarily seem like a good idea. We have > no guarantee at all that the index scan will be faster than scanning > the table would have been, and a single table scan can do multiple > verification steps if, for example, multiple columns are set NOT NULL > at the same time. Isn't the first concern addressed by using SPI..? As for the second concern, couldn't that be done with a more complicated query through SPI, though things might have to be restructured some to make it possible to do that. Just, generally speaking, this is definitely something that I think we want and neither of the above concerns seem like they're technical reasons why we can't use something like this approach, just needs to perhaps be reworked to handle multiple columns in a single query. Thanks! Stephen
Stephen Frost <sfrost@snowman.net> writes:
> Isn't the first concern addressed by using SPI..?
I did not look at the patch yet, but TBH if it uses SPI for sub-operations
of ALTER TABLE I think that is sufficient reason to reject it out of hand.
Doing things that way would create way too much of a vulnerability surface
for code touching a partially-updated table. At minimum, we'd have to
blow holes in existing protections like CheckTableNotInUse, and I think
we'd be forever finding other stuff that failed to work quite right in
that context. I do not want ALTER TABLE going anywhere near the planner
or executor; I'm not even happy that it uses the parser (for index
definition reconstruction).
regards, tom lane
Thanks all for reply! Robert, > Doing it based on an index scan doesn't necessarily seem like a good idea. We have > no guarantee at all that the index scan will be faster than scanning > the table would have been I agree this. Thinking a little about idea of index scan i can not give reasonable usecase which required index. My targetproblem of adding NOT NULL to big relation without long downtime can be done with ADD CONSTRAINT NOT VALID, VALIDATEit in second transaction, then SET NOT NULL by my patch and drop unneeded constraint. Stephen, > Just, generally speaking, this is definitely something that I think we > want and neither of the above concerns seem like they're technical > reasons why we can't use something like this approach, just needs to > perhaps be reworked to handle multiple columns in a single query. I understood the idea, thank you. Tom, > I did not look at the patch yet, but TBH if it uses SPI for sub-operations > of ALTER TABLE I think that is sufficient reason to reject it out of hand. I understood, thank you. So, i will soon delete SPI usage and index scan and post new simplified patch with verify data only by constraints.
Tom, * Tom Lane (tgl@sss.pgh.pa.us) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > Isn't the first concern addressed by using SPI..? > > I did not look at the patch yet, but TBH if it uses SPI for sub-operations > of ALTER TABLE I think that is sufficient reason to reject it out of hand. You mean like what ALTER TABLE ... ADD FOREIGN KEY does? > Doing things that way would create way too much of a vulnerability surface > for code touching a partially-updated table. At minimum, we'd have to > blow holes in existing protections like CheckTableNotInUse, and I think > we'd be forever finding other stuff that failed to work quite right in > that context. I do not want ALTER TABLE going anywhere near the planner > or executor; I'm not even happy that it uses the parser (for index > definition reconstruction). That's more along the lines of the kind of response I was expecting given the suggestion, and perhaps a good reason to just go with the index-based lookup, when an index is available to do so with, but I'm not entirely sure how this is different from how we handle foreign keys. Thanks! Stephen
On Wed, Nov 29, 2017 at 10:52 AM, Sergei Kornilov <sk@zsrv.org> wrote: > I agree this. Thinking a little about idea of index scan i can not give reasonable usecase which required index. My targetproblem of adding NOT NULL to big relation without long downtime can be done with ADD CONSTRAINT NOT VALID, VALIDATEit in second transaction, then SET NOT NULL by my patch and drop unneeded constraint. Yes, I had the same thought. Thinking a little bit further, maybe the idea you had in mind using the index scan was that some indexes offer cheap ways of testing whether ANY nulls are present in the column. For example, if we had a non-partial btree index whose first column is the one being made NOT NULL, we could try an index scan - via index_beginscan() / index_getnext() / index_endscan() - trying to pull exactly one null from the index, and judge whether or not there are nulls present based only whether we get one. This would be a lot cheaper than scanning a large table, but it needs some careful thought because of visibility issues. It's not sufficient that the index contains no nulls that are visible to our snapshot; it must contain no nulls that are visible to any plausible current or future snapshot. I doubt that test can be written in SQL, but it can probably be written in C. Moreover, we need to avoid not only false negatives (thinking that there is no NULL when there is one) but also false positives (thinking there's a NULL in the column when there isn't, and thus failing spuriously). But it seems like it might be useful if someone can figure out the details of how to make it 100% correct; one index lookup is sure to be a lot quicker than a full table scan. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I did not look at the patch yet, but TBH if it uses SPI for sub-operations
>> of ALTER TABLE I think that is sufficient reason to reject it out of hand.
> You mean like what ALTER TABLE ... ADD FOREIGN KEY does?
Yeah, and if you look at the warts that SPI has grown to support that
usage, you'll see why I'm so unhappy. We should never have allowed
FKs to be built on top of SPI; they require semantics that don't exist
in SQL. I think this would lead to more of the same --- not exactly
the same of course, but more warts. See Robert's nearby musings about
semantics of index null checks for an example.
regards, tom lane
Robert, * Robert Haas (robertmhaas@gmail.com) wrote: > On Wed, Nov 29, 2017 at 10:52 AM, Sergei Kornilov <sk@zsrv.org> wrote: > > I agree this. Thinking a little about idea of index scan i can not give reasonable usecase which required index. My targetproblem of adding NOT NULL to big relation without long downtime can be done with ADD CONSTRAINT NOT VALID, VALIDATEit in second transaction, then SET NOT NULL by my patch and drop unneeded constraint. > > Yes, I had the same thought. I have to admit that the case I was thinking about was the one you outline below.. > Thinking a little bit further, maybe the idea you had in mind using > the index scan was that some indexes offer cheap ways of testing > whether ANY nulls are present in the column. For example, if we had a > non-partial btree index whose first column is the one being made NOT > NULL, we could try an index scan - via index_beginscan() / > index_getnext() / index_endscan() - trying to pull exactly one null > from the index, and judge whether or not there are nulls present based > only whether we get one. This would be a lot cheaper than scanning a > large table, but it needs some careful thought because of visibility > issues. It's not sufficient that the index contains no nulls that are > visible to our snapshot; it must contain no nulls that are visible to > any plausible current or future snapshot. I doubt that test can be > written in SQL, but it can probably be written in C. Moreover, we > need to avoid not only false negatives (thinking that there is no NULL > when there is one) but also false positives (thinking there's a NULL > in the column when there isn't, and thus failing spuriously). But it > seems like it might be useful if someone can figure out the details of > how to make it 100% correct; one index lookup is sure to be a lot > quicker than a full table scan. This was along the lines I was thinking also (though I had thought SPI might be reasonable given our usage of it with FKs, but if it'd require uglier warts than what SPI already has, then just finding an appropriate index and using the internal methods would be best). As for conflicting snapshots, isn't the lock we're taking already AccessExclusive..? Thanks! Stephen
Here is new patch with check only existed valid constraints and without SPI at all. Thanks
Attachment
On November 29, 2017 8:50:31 AM PST, Stephen Frost <sfrost@snowman.net> wrote: >As for conflicting snapshots, isn't the lock we're taking already >AccessExclusive..? Doesn't help if e.g. the current xact is repeatable read or if your own xact deleted things (other xacts with snapshots couldstill see null rows, despite new take definition). Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Wed, Nov 29, 2017 at 11:53 AM, Sergei Kornilov <sk@zsrv.org> wrote:
> Here is new patch with check only existed valid constraints and without SPI at all.
Please use pgindent or anyway try to follow project style. { needs to
go on a line by itself, for example.
isSetNotNullNeedsTableScan seems different than other similarly-named
functions we have. NotNullImpliedByRelConstraints?
It also lacks a header comment.
Having both PartConstraintImpliedByRelConstraint and
ConstraintImpliedByRelConstraint with identical implementations
doesn't seem like a good idea to me. I guess just renaming it is
probably fine.
The comment /* T if we added new NOT NULL constraints */ should
probably be changed to /* T if we should recheck NOT NULL constraints
*/ or similar.
I'd suggest registering this with the next CommitFest.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hello I update patch and also rebase to current head. I hope now it is better aligned with project style
Attachment
Greetings Sergei,
* Sergei Kornilov (sk@zsrv.org) wrote:
> I update patch and also rebase to current head. I hope now it is better aligned with project style
Thanks for updating it and continuing to work on it. I haven't done a
full review but there were a few things below that I thought could be
improved-
> @@ -5863,8 +5864,11 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
>
> CatalogTupleUpdate(attr_rel, &tuple->t_self, tuple);
>
> - /* Tell Phase 3 it needs to test the constraint */
> - tab->new_notnull = true;
> + if (!NotNullImpliedByRelConstraints(rel, (Form_pg_attribute) GETSTRUCT(tuple)))
> + {
> + /* Tell Phase 3 it needs to test the constraint */
> + tab->verify_new_notnull = true;
> + }
>
> ObjectAddressSubSet(address, RelationRelationId,
> RelationGetRelid(rel), attnum);
This could really use some additional comments, imv. In particular, we
need to make it clear that verify_new_notnull only moves from the
initial 'false' value to 'true', since we could be asked to add multiple
NOT NULL constraints and if any of them aren't already covered by an
existing CHECK constraint then we need to perform the full table check.
> @@ -13618,15 +13662,14 @@ ComputePartitionAttrs(Relation rel, List *partParams, AttrNumber *partattrs,
> }
>
> /*
> - * PartConstraintImpliedByRelConstraint
> - * Does scanrel's existing constraints imply the partition constraint?
> + * ConstraintImpliedByRelConstraint
> + * Does scanrel's existing constraints imply given constraint
> *
> * Existing constraints includes its check constraints and column-level
> * NOT NULL constraints and partConstraint describes the partition constraint.
> */
> bool
> -PartConstraintImpliedByRelConstraint(Relation scanrel,
> - List *partConstraint)
> +ConstraintImpliedByRelConstraint(Relation scanrel, List *partConstraint)
> {
> List *existConstraint = NIL;
> TupleConstr *constr = RelationGetDescr(scanrel)->constr;
I would also rename 'partConstraint' since this function is no longer,
necessairly, working with a partition's constraint.
This could also really use some regression tests and I'd be sure to
include tests of adding multiple NOT NULL constraints, sometimes where
they're all covered by existing CHECK constraints and other times when
only one or the other is (and there's existing NULL values in the other
column), etc.
Thanks!
Stephen
Attachment
Hello Stephen I changed the suggested things and added some regression tests. Also i wrote few words to the documentation. New patch isattached. Thank you for feedback! regards, Sergei
Attachment
Hello Sergei, I couldn't find any case when your code doesn't work properly. So it seems ok to me. > @@ -220,6 +220,13 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM > </para> > > <para> > + Full table scan is performed to check that no existing row > + in the table has null values in given column. It is possible to avoid > + this scan by adding a valid <literal>CHECK</literal> constraint to > + the table that would allow only NOT NULL values for given column. > + </para> Adding check constraint will also force the full table scan. So I think it would be better to rephrased it as follows: "Full table scan is performed to check that column doesn't contain NULL values unless there are valid check constraints that prohibit NULL values for specified column. In the latter case table scan is skipped." A native English speaker input would be helpful here. Regarding regression tests it may be useful to set client_min_messages to 'debug1' before setting "NOT NULL" attribute for a column. In this case you can tell for sure that NotNullImpliedByRelConstraints() returned true (i.e. your code actually did the job) as the special debug message is printed to the log. Thanks! -- Ildar Musin i.musin@postgrespro.ru
Hello thank you for review! > Adding check constraint will also force the full table scan. So I think > it would be better to rephrased it as follows: Agree. I updated docs in new attached patch slightly different > Regarding regression tests it may be useful to set client_min_messages > to 'debug1' before setting "NOT NULL" attribute for a column. In this > case you can tell for sure that NotNullImpliedByRelConstraints() > returned true (i.e. your code actually did the job) as the special debug > message is printed to the log. I can not find any debug messages in tests: grep client_min_messages -irn src/test/ Only some warning level and few error.So i verify in regression tests only top-level behavior. Or this paragraph was for other people, not for tests? regards, Sergei
Attachment
On 06.03.2018 16:12, Sergei Kornilov wrote: > Hello thank you for review! > >> Adding check constraint will also force the full table scan. So I >> think it would be better to rephrased it as follows: > Agree. I updated docs in new attached patch slightly different > >> Regarding regression tests it may be useful to set >> client_min_messages to 'debug1' before setting "NOT NULL" attribute >> for a column. In this case you can tell for sure that >> NotNullImpliedByRelConstraints() returned true (i.e. your code >> actually did the job) as the special debug message is printed to >> the log. > I can not find any debug messages in tests: grep client_min_messages > -irn src/test/ Only some warning level and few error. So i verify in > regression tests only top-level behavior. Or this paragraph was for > other people, not for tests? > Sorry, probably I didn't explain it clearly enough. I meant that your regression tests can't distinguish cases when the full table scan was actually performed from the ones when it was skipped due to NotNullImpliedByRelConstraints() check. For instance, consider this piece from the test suite: # create table atacc1 (test_a int, test_b int); CREATE TABLE ... # alter table atacc1 add constraint atacc1_constr_a_valid check(test_a is not null); ALTER TABLE # alter table atacc1 alter test_a set not null; ALTER TABLE It is not obvious that full table scan was omitted. But if we set client_min_messages to 'debug1', we'll be able to see what is really happened by the different debug messages. For example: # create table atacc1 (test_a int, test_b int); CREATE TABLE ... # set client_min_messages to 'debug1'; SET # alter table atacc1 alter test_a set not null; DEBUG: verifying table "atacc1" <<<< full table scan! ALTER TABLE # alter table atacc1 alter test_a drop not null; ALTER TABLE # alter table atacc1 add constraint atacc1_constr_a_valid check(test_a is not null); DEBUG: verifying table "atacc1" ALTER TABLE # alter table atacc1 alter test_a set not null; DEBUG: verifying table "atacc1" NOT NULL constraint on test_a attribute by existed constraints <<<< full scan was skipped! ALTER TABLE -- Ildar Musin i.musin@postgrespro.ru
Hello, Ildar Thanks. I looked ATTACH PARTITION tests and found such checks. If no one is against i will just use in my patch INFO levelinstead DEBUG1, similarly ATTACH PARTITION code. Updated patch attached. Or i can rewrite tests to use DEBUG1 level. regards, Sergei
Attachment
Sergei Kornilov wrote: > Hello, Ildar > Thanks. I looked ATTACH PARTITION tests and found such checks. If no one is against i will just use in my patch INFO levelinstead DEBUG1, similarly ATTACH PARTITION code. Updated patch attached. > > Or i can rewrite tests to use DEBUG1 level. You should be able to use an event trigger that raises a message when table_rewrite is hit, to notify the test driver that a rewrite happens. (If any DDL that causes a table rewrite fails to trigger the table_rewrite event correctly, that is a bug.) ISTM that depending on DEBUG messages is bad because debugging lines added elsewhere will make your tests fail; and INFO is generally frowned upon (I, for one, would frown upon an INFO message raised by ALTER TABLE, for sure.) so let's not do that either. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello > You should be able to use an event trigger that raises a message when > table_rewrite is hit, to notify the test driver that a rewrite happens. Here is no table rewrite, only verify. So here EventTriggerTableRewrite is not called. Or i missed something? > ISTM that depending on DEBUG messages is bad because debugging lines > added elsewhere will make your tests fail; I agree and this is reason why i not used DEBUG message in tests as was proposed. I found INFO messages in tests and decidedthat this was an acceptable option year ago. > and INFO is generally frowned upon (I, for one, would frown upon an INFO message raised by ALTER > TABLE, for sure.) so let's not do that either. In this case we need remove INFO from ALTER TABLE ATTACH PARTITION code and tests. I think is a bad idea of using differentrules for same stuff, right? Probably i can do this work too. But it is necessary to decide how the test should look. regards, Sergei
Sergei Kornilov <sk@zsrv.org> writes:
>> ISTM that depending on DEBUG messages is bad because debugging lines
>> added elsewhere will make your tests fail;
> I agree and this is reason why i not used DEBUG message in tests as was proposed. I found INFO messages in tests and
decidedthat this was an acceptable option year ago.
INFO is for user-facing messages, such as output from VACUUM VERBOSE,
where the user has specifically requested the output. I do not think
this falls into that category. Even if you persuade some committer
to commit it like that, there will be user pushback asking us to get
this noise out of their faces ... and then we'll have to find some
other way to test it.
Do you actually need test output proving that this code path was taken
rather than the default one? Seems like looking at the code coverage
report might be enough.
> In this case we need remove INFO from ALTER TABLE ATTACH PARTITION code and tests. I think is a bad idea of using
differentrules for same stuff, right? Probably i can do this work too.
I did not see any INFO messages in a quick test of ALTER TABLE ATTACH
PARTITION, but if there are any lurking in there, they probably need
to be downgraded.
regards, tom lane
Hello > Do you actually need test output proving that this code path was taken > rather than the default one? Seems like looking at the code coverage > report might be enough. I not known. In v4 i use DEBUG1 message and do not check code path in tests at all: by full table scan or by constraint,i tested only command result to not break behavior. Today Ildar Musin proposed to test code path through NotNullImpliedByRelConstraints function. This is my first patch andI do not have the confidence to write a test. So I looked more closely at the alter table tests, found several info inattach partition and updated my patch. > I did not see any INFO messages in a quick test of ALTER TABLE ATTACH > PARTITION, but if there are any lurking in there, they probably need > to be downgraded. In src/test/regress/expected/alter_table.out i found 7 test with > INFO: partition constraint for table "..." is implied by existing constraints and 5 with > INFO: updated partition constraint for default partition "..." is implied by existing constraints ereport's are in ValidatePartitionConstraints function src/backend/commands/tablecmds.c regards, Sergei
Hello Sergei, Alvaro, Tom, On 06.03.2018 20:25, Alvaro Herrera wrote: > You should be able to use an event trigger that raises a message when > table_rewrite is hit, to notify the test driver that a rewrite happens. > > (If any DDL that causes a table rewrite fails to trigger the > table_rewrite event correctly, that is a bug.) > It seems that 'table_rewrite' event trigger isn't fired in case of simple scan (without actual rewrite). > ISTM that depending on DEBUG messages is bad because debugging lines > added elsewhere will make your tests fail; I think that between test depending on DEBUG message and no test at all the former one is preferable. Are there other options to test it? On 06.03.2018 21:51, Tom Lane wrote: > > Do you actually need test output proving that this code path was taken > rather than the default one? Seems like looking at the code coverage > report might be enough. > Code coverage cannot guarantee correctness. I think there should be at least two tests: * full scan is performed when there are no check constraints that restrict NULL values; * full scan is omitted when there are such constraints. The last one could also include some unobvious cases like CHECK (col > 0), complex expressions with boolean operators etc. PS: Btw, some check constraints that implies NOT NULL still won't prevent from full table scan. For instance: # create table test (a int, b int check ((a + b) is not null)); CREATE TABLE # set client_min_messages to 'debug1'; SET # insert into test values (1, 1); INSERT 0 1 # alter table test alter column a set not null; DEBUG: verifying table "test" <<<< full table scan! ALTER TABLE Since operator+ (aka int4pl) is strict it returns NULL if at least one of its operands is NULL. And this check constraint ensures therefore that both columns 'a' and 'b' are NOT NULL. It isn't probably the issue of this patch, just something that someone may find interesting. -- Ildar Musin i.musin@postgrespro.ru
Hello all! Summarizing, how i must update tests? We want test real skip of verify phase or only result correctness? I can verify what i do not break alter table by tests in v4 of my patch. But here is no check code path. So my feature maybe broken in future without test fail. alter table will be correct regardless this feature. Or i must test code path too? I understood Tom and will remove INFO ereport from patch anyway. But currently i have no ideahow check code path. As mentioned earlier using debug ereport is uncomfortable too. Here is no suitable event triggerto track what exactly happens with table. Adding new one for test purposes seems overkill. regards, Sergei
Hello My patch does not apply after commit 5748f3a0aa7cf78ac6979010273bd9d50869bb8e. Here is update to current master. Not nullconstraint is immutable too, so here is no changes in PartConstraintImpliedByRelConstraint excepts rename and commentsfix. In this patch version i also revert tests to v4 state: i use DEBUG ereport instead INFO and code path not tested. Pleasetell me if i must change tests some way. regards, Sergei
Attachment
Hello Sergei, On 10.03.2018 12:35, Sergei Kornilov wrote: > Hello My patch does not apply after commit > 5748f3a0aa7cf78ac6979010273bd9d50869bb8e. Here is update to current > master. Not null constraint is immutable too, so here is no changes > in PartConstraintImpliedByRelConstraint excepts rename and comments > fix. > > In this patch version i also revert tests to v4 state: i use DEBUG > ereport instead INFO and code path not tested. Please tell me if i > must change tests some way. > > regards, Sergei > Ok, I can't think of any other ways to test it so I have to agree with Tom Lane i.e. rely only on coverage. (There also were another suggestion to use statistics [select seq_scan from pg_stat_user_tables where relid='test'::regclass] which show number of table scans. But statistics is collected by stat collector process with some latency and hence cannot be reliable for tests). Patch seems correct to me, it applies and compiles cleanly, docs compiles as well, tests pass. Changed status to Ready for Committer. -- Ildar Musin i.musin@postgrespro.ru
On 11/29/17 10:52, Sergei Kornilov wrote: > My target problem of adding NOT NULL to big relation without long downtime can be done with ADD CONSTRAINT NOT VALID, VALIDATEit in second transaction, then SET NOT NULL by my patch and drop unneeded constraint. It seems to me that this is a workaround for not properly cataloguing NOT NULL constraints. If we fixed that, you could do SET NOT NULL NOT VALID and go from there. Maybe we should look again into fixing that. That would solve so many problems. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello, Peter! I agree my patch seems as strange workaround. And better will be SET NOT NULL NOT VALID feature. But this change is verycomplicated for me For now my patch is small, simple and provides way for users. regards, Sergei 06.04.2018, 06:29, "Peter Eisentraut" <peter.eisentraut@2ndquadrant.com>: > On 11/29/17 10:52, Sergei Kornilov wrote: >> My target problem of adding NOT NULL to big relation without long downtime can be done with ADD CONSTRAINT NOT VALID,VALIDATE it in second transaction, then SET NOT NULL by my patch and drop unneeded constraint. > > It seems to me that this is a workaround for not properly cataloguing > NOT NULL constraints. If we fixed that, you could do SET NOT NULL NOT > VALID and go from there. Maybe we should look again into fixing that. > That would solve so many problems. > > -- > Peter Eisentraut http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hello I notice my patch does not apply again. Here is update to current HEAD regards, Sergei
Attachment
Hi I noticed new merge conflict, updated version attached. regards, Sergei
Attachment
Hello Attached updated patch follows recent Reorganize partitioning code commit. regards, Sergei
Attachment
>On Sun, 15 Apr 2018 at 09:09, Sergei Kornilov <sk@zsrv.org> wrote: > > Attached updated patch follows recent Reorganize partitioning code commit. > regards, Sergei This patch went through the last tree commit fests without any noticeable activity, but cfbot says it still applies and doesn't break any tests. The patch itself is rather small, the only significant objection I see from the thread is that probably "SET NOT NULL NOT VALID" feature could be more appropriate way of fixing the original problem, but looks like no one is working on that (the only related thread I could found was this one [1]). If not properly cataloguing NOT NULL constraints would be fixed, can it potentially conflict with the current patch or not? [1]: https://www.postgresql.org/message-id/flat/CAASwCXcOokBqHf8BXECbDgOLkXxJyL_Q_twhg2dGWSvgMzz%3DxA%40mail.gmail.com
Hello > This patch went through the last tree commit fests without any noticeable activity Well, last two CF. During march commitfest patch has activity and was marked as ready for committer. But at end of july CFwas no further activity and patch was reverted back to "need review" with reason [1]: > It's not clear that there is consensus that this is wanted. I can't say something here. > If not properly cataloguing NOT NULL constraints would be fixed, can it > potentially conflict with the current patch or not? We already doing same stuff for "alter table attach partition" and in this patch i use exactly this routine. If proper cataloguingwould conflict with my patch - it would conflict with "attach partition" validation too. I think proper cataloguing can be implemented without conflict with proposed feature. regards, Sergei [1]: https://www.postgresql.org/message-id/c4c7556d-a046-ac29-2549-bdef0078b6fe%402ndQuadrant.com
> On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov <sk@zsrv.org> wrote: > > > If not properly cataloguing NOT NULL constraints would be fixed, can it > > potentially conflict with the current patch or not? > We already doing same stuff for "alter table attach partition" and in this patch i use exactly this routine. If propercataloguing would conflict with my patch - it would conflict with "attach partition" validation too. > I think proper cataloguing can be implemented without conflict with proposed feature. Yes, indeed, this patch relies on the PartConstraintImpliedByRelConstraint. Then maybe it makes sense to go with the solution, proposed in this thread, while leaving open the possibility of having "SET NOT NULL NOT VALID"? From the functionality point of view it definitely would be beneficial. Any other opinions?
On 2018-Nov-13, Dmitry Dolgov wrote: > > On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov <sk@zsrv.org> wrote: > > > > > If not properly cataloguing NOT NULL constraints would be fixed, can it > > > potentially conflict with the current patch or not? > > We already doing same stuff for "alter table attach partition" and in this patch i use exactly this routine. If propercataloguing would conflict with my patch - it would conflict with "attach partition" validation too. > > I think proper cataloguing can be implemented without conflict with proposed feature. > > Yes, indeed, this patch relies on the PartConstraintImpliedByRelConstraint. > Then maybe it makes sense to go with the solution, proposed in this thread, > while leaving open the possibility of having "SET NOT NULL NOT VALID"? From > the functionality point of view it definitely would be beneficial. Any other > opinions? I think we should ignore any SET NOT NULL NOT VALID patches, because in practice they don't exist. Let's move forward with this, because the optimization being proposed is clearly very useful. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On Tue, Nov 13, 2018 at 1:59 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2018-Nov-13, Dmitry Dolgov wrote: > > > > On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov <sk@zsrv.org> wrote: > > > > > > > If not properly cataloguing NOT NULL constraints would be fixed, can it > > > > potentially conflict with the current patch or not? > > > We already doing same stuff for "alter table attach partition" and in this patch i use exactly this routine. If propercataloguing would conflict with my patch - it would conflict with "attach partition" validation too. > > > I think proper cataloguing can be implemented without conflict with proposed feature. > > > > Yes, indeed, this patch relies on the PartConstraintImpliedByRelConstraint. > > Then maybe it makes sense to go with the solution, proposed in this thread, > > while leaving open the possibility of having "SET NOT NULL NOT VALID"? From > > the functionality point of view it definitely would be beneficial. Any other > > opinions? > > I think we should ignore any SET NOT NULL NOT VALID patches, because > in practice they don't exist. Let's move forward with this, because the > optimization being proposed is clearly very useful. Absolutely agree. Looking at the history of the patch I see that it went through some extensive review and even was marked as Ready for Committer before the commentary from Peter, but since then some changes were made (rebase and code reorganization). Can someone from the reviewers confirm (or not) if the patch is in a good shape? If no, then I'll try to allocate some time for that, but can't promise any exact date.
> On Thu, Nov 22, 2018 at 6:04 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> Absolutely agree. Looking at the history of the patch I see that it went
> through some extensive review and even was marked as Ready for Committer before
> the commentary from Peter, but since then some changes were made (rebase and
> code reorganization). Can someone from the reviewers confirm (or not) if the
> patch is in a good shape? If no, then I'll try to allocate some time for that,
> but can't promise any exact date.
Ok, I've reviewed this patch a bit. I don't see any significant problems with
the code itself, but to be honest I've got an impression that simplicity of
this patch is misleading and it covers too narrow use case. E.g. one can do
something equal to SET NOT NULL, like adding a new constraint
CREATE TABLE test(data text, CHECK(data IS NOT NULL));
ALTER TABLE test ADD CONTSTRAINT data_chk CHECK (data is not null);
or use not null domain
CREATE DOMAIN not_null AS text CHECK(VALUE IS NOT NULL);
CREATE TABLE test(data not_null);
ALTER TABLE test ALTER COLUMN data SET NOT NULL;
and it still leads to a full table scan. For foreign tables the control flow
jump over NotNullImpliedByRelConstraints, so this logic is never checked. I'm
not sure if taking care of mentioned examples is enough (maybe there are more
of them), or the patch needs to suggest some more general solution here.
Of course there is a possibility that improvement even in this form for this
narrow use case is better than potentially more general approach in the future.
Any opinions about this?
On Sun, 15 Apr 2018 at 19:09, Sergei Kornilov <sk@zsrv.org> wrote: > Attached updated patch follows recent Reorganize partitioning code commit. I've made a pass over v10. I think it's in pretty good shape, but I did end up changing a few small things. 1. Tweaked the documents a bit. I was going to just change "Full table scan" to "A full table scan", but I ended up rewriting the entire paragraph. 2. In ATRewriteTables, updated comment to mention "If required" since the scan may not be required if SET NOT NULLs are already proved okay. 3. Updated another forgotten comment in ATRewriteTables. 4. Removed mention about table's with OIDs in ATExecAddColumn(). This seems left over from 578b229718. 5. Changed ATExecSetNotNull not to check for matching constraints if we're already going to make a pass over the table. Setting verify_new_notnull to true again does not make it any more true. :) 6. Tweaked NotNullImpliedByRelConstraints() a bit. You were calling lappend on an empty list. make_list1() is fine. Don't need a variable for that, just pass it to the function. 7. Tightened up the comments in ConstraintImpliedByRelConstraint() to mention that it only supports immutable quals containing vars with varno=1. Removed the comment near the bottom that mentioned that in passing. The only thing that I'm a bit unsure of is the tests. I've read the thread and I see the discussion above about it. I'd personally have thought INFO was fine since ATTACH PARTITION does that, but I see objections. It appears that all the tests just assume that the CHECK constraint was used to validate the SET NOT NULL. I'm not all that sure if there's any good reason not to set client_min_messages = 'debug1' just before your test then RESET client_min_messages; afterwards. No other tests seem to do it, but my only thoughts on the reason not to would be that it might fail if someone added another debug somewhere, but so what? they should update the expected results too. Oh, one other thing I don't really like is that ConstraintImpliedByRelConstraint() adds all of the other column's IS NOT NULL constraints to the existConstraint. This is always wasted effort for the SET NOT NULL use case. I wonder if a new flag should be added to control this, or even better, separate that part out and put back the function named PartConstraintImpliedByRelConstraint and have it do the IS NOT NULL building before calling ConstraintImpliedByRelConstraint(). No duplicate code that way and you also don't need to touch partbound.c Setting this on waiting on author. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
Hello, David! > I've made a pass over v10. I think it's in pretty good shape, but I > did end up changing a few small things. Thank you! I merged your changes to new patch version. > The only thing that I'm a bit unsure of is the tests. I've read the > thread and I see the discussion above about it. I'd personally have > thought INFO was fine since ATTACH PARTITION does that, but I see > objections. It is unclear for me if we have consensus about INFO ereport in ATTACH PARTITION. > It appears that all the tests just assume that the CHECK > constraint was used to validate the SET NOT NULL. I'm not all that > sure if there's any good reason not to set client_min_messages = > 'debug1' just before your test then RESET client_min_messages; > afterwards. No other tests seem to do it, but my only thoughts on the > reason not to would be that it might fail if someone added another > debug somewhere, but so what? they should update the expected results > too. Not sure we have consensus about debug messages in tests, so I did such test changes in additional patch. > separate that part out and > put back the function named PartConstraintImpliedByRelConstraint and > have it do the IS NOT NULL building before calling > ConstraintImpliedByRelConstraint(). No duplicate code that way and you > also don't need to touch partbound.c In this case we need extra argument for ConstraintImpliedByRelConstraint for some caller-provided existConstraint, right?Along with Relation itself? Then I need make copy of existConstraint, append relation constraints and call predicate_implied_by.If I understood correctly pseudocode would be: PartConstraintImpliedByRelConstraint(rel, testConstraint) generate notNullConstraint from NOT NULL table attributes call ConstraintImpliedByRelConstraint(rel, testConstraint, notNullConstraint) ConstraintImpliedByRelConstraint(rel, testConstraint, existsConstraint) copy existsConstraint to localExistsConstraint variable append relation constraints to localExistsConstraint list call predicate_implied_by I thing redundant IS NOT NULL check is not issue here and we not need extra arguments for ConstraintImpliedByRelConstraint.Was not changed in attached version, but I will change if you think this would be betterdesign. regards, Sergei
Attachment
On Wed, 6 Mar 2019 at 08:41, Sergei Kornilov <sk@zsrv.org> wrote: > In this case we need extra argument for ConstraintImpliedByRelConstraint for some caller-provided existConstraint, right?Along with Relation itself? Then I need make copy of existConstraint, append relation constraints and call predicate_implied_by.If I understood correctly pseudocode would be: > > PartConstraintImpliedByRelConstraint(rel, testConstraint) > generate notNullConstraint from NOT NULL table attributes > call ConstraintImpliedByRelConstraint(rel, testConstraint, notNullConstraint) > > ConstraintImpliedByRelConstraint(rel, testConstraint, existsConstraint) > copy existsConstraint to localExistsConstraint variable > append relation constraints to localExistsConstraint list > call predicate_implied_by > > I thing redundant IS NOT NULL check is not issue here and we not need extra arguments for ConstraintImpliedByRelConstraint.Was not changed in attached version, but I will change if you think this would be betterdesign. I was really just throwing the idea out there for review. I don't think that I'm insisting on it. FWIW I think you could get away without the copy of the constraint providing it was documented that the list is modified during the ConstraintImpliedByRelConstraint call and callers must make a copy themselves if they need an unmodified version. Certainly, PartConstraintImpliedByRelConstraint won't need to make a copy, so there's probably not much reason to assume that possible future callers will. Providing I'm imagining it correctly, I do think the patch is slightly cleaner with that change. It's: a) slightly more efficient due to not needlessly checking a bunch of IS NOT NULLs (imagine a 1000 column table with almost all NOT NULL and a single CHECK constraint); and b) patch has a smaller footprint (does not modify existing callers of PartConstraintImpliedByRelConstraint()); and c) does not modify an existing API function. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Hi > Providing I'm imagining it correctly, I do think the patch is slightly > cleaner with that change. Ok, sounds reasonable. I changed patch this way. regards, Sergei
Attachment
On Mon, 11 Mar 2019 at 00:13, Sergei Kornilov <sk@zsrv.org> wrote: > > > Providing I'm imagining it correctly, I do think the patch is slightly > > cleaner with that change. > > Ok, sounds reasonable. I changed patch this way. Looks good to me. Good idea to keep the controversial setting of client_min_messages to debug1 in the tests in a separate patch. Apart from a few lines that need to be wrapped at 80 chars and some comment lines that seem to have been wrapped early, I'm happy for it to be marked as ready for committer, but I'll defer to Ildar in case he wants to have another look. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Sun, Mar 10, 2019 at 11:30 PM David Rowley
<david.rowley@2ndquadrant.com> wrote:
> Looks good to me. Good idea to keep the controversial setting of
> client_min_messages to debug1 in the tests in a separate patch.
>
> Apart from a few lines that need to be wrapped at 80 chars and some
> comment lines that seem to have been wrapped early, I'm happy for it
> to be marked as ready for committer, but I'll defer to Ildar in case
> he wants to have another look.
Dispatches from the department of grammatical nitpicking...
+ entire table, however if a valid <literal>CHECK</literal> constraint is
I think this should be:
entire table; however, if...
+ * are set NOT NULL, however, if we can find a constraint which proves
similarly here
+ ereport(DEBUG1,
+ (errmsg("verifying table \"%s\" NOT NULL constraint "
+ "on %s attribute by existed constraints",
+ RelationGetRelationName(rel), NameStr(attr->attname))));
Ugh, that doesn't read well at all. How about:
existing constraints on column "%s"."%s" are sufficient to prove that
it does not contain nulls
- * in implicit-AND form.
+ * in implicitly-AND form, must only contain immutable clauses
+ * and all Vars must be varno=1.
I think you should leave the existing sentence alone (implicit-AND is
correct, implicitly-AND is not) and add a new sentence that says the
stuff you want to add.
+ * "Existing constraints" include its check constraints and optional
+ * caller-provided existConstraint list. existConstraint list is modified
+ * during ConstraintImpliedByRelConstraint call and would represent all
+ * assumed conditions. testConstraint describes the constraint to validate.
+ * Both existConstraint and testConstraint must be in implicitly-AND form,
+ * must only contain immutable clauses and all Vars must be varno=1.
I think that it might be better to copy the list rather than to have
the comment note that it gets mutated, but regardless the grammar
needs improvement here. Maybe: On entry, existConstraint is a
caller-provided list of conditions which this function may assume to
be true; on exit, it will have been destructively modified by the
addition of the table's CHECK constraints. testConstraint is the
constraint to validate. Both existConstraint and testConstraint must
be in implicit-AND form, must only contain immutable clauses, and must
contain only Vars with varno = 1.
- * not-false and try to prove the same for partConstraint.
- *
- * Note that predicate_implied_by assumes its first argument is known
- * immutable. That should always be true for partition constraints, so we
- * don't test it here.
+ * not-false and try to prove the same for testConstraint.
I object to removing this.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Hello
> Dispatches from the department of grammatical nitpicking...
Thank you!
> + entire table, however if a valid <literal>CHECK</literal> constraint is
>
> I think this should be:
>
> entire table; however, if...
>
> + * are set NOT NULL, however, if we can find a constraint which proves
>
> similarly here
Changed
> + ereport(DEBUG1,
> + (errmsg("verifying table \"%s\" NOT NULL constraint "
> + "on %s attribute by existed constraints",
> + RelationGetRelationName(rel), NameStr(attr->attname))));
>
> Ugh, that doesn't read well at all. How about:
>
> existing constraints on column "%s"."%s" are sufficient to prove that
> it does not contain nulls
Changed
> - * in implicit-AND form.
> + * in implicitly-AND form, must only contain immutable clauses
> + * and all Vars must be varno=1.
>
> I think you should leave the existing sentence alone (implicit-AND is
> correct, implicitly-AND is not) and add a new sentence that says the
> stuff you want to add.
Ok
> + * "Existing constraints" include its check constraints and optional
> + * caller-provided existConstraint list. existConstraint list is modified
> + * during ConstraintImpliedByRelConstraint call and would represent all
> + * assumed conditions. testConstraint describes the constraint to validate.
> + * Both existConstraint and testConstraint must be in implicitly-AND form,
> + * must only contain immutable clauses and all Vars must be varno=1.
>
> I think that it might be better to copy the list rather than to have
> the comment note that it gets mutated, but regardless the grammar
> needs improvement here.
Agreed, in attached new version I copy the list and do not modify parameter.
> I object to removing this.
Okay, I revert this David's change
regards, Sergei
Attachment
On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov <sk@zsrv.org> wrote: > Agreed, in attached new version ... Committed with a little more nitpicking. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Wow, thank you! 13.03.2019, 15:57, "Robert Haas" <robertmhaas@gmail.com>: > On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov <sk@zsrv.org> wrote: >> Agreed, in attached new version ... > > Committed with a little more nitpicking. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov <sk@zsrv.org> wrote:
>> Agreed, in attached new version ...
> Committed with a little more nitpicking.
The buildfarm thinks additional nitpicking is needed.
regards, tom lane
Hi > The buildfarm thinks additional nitpicking is needed. hm. Patch was committed with debug1 level tests and many animals uses log_statement = 'all'. Therefore they have additionalline in result: LOG: statement: alter table pg_class alter column relname drop not null; and similar for otherqueries. I think we better would be to revert "set client_min_messages to 'debug1';" part. regards, Sergei
On Wed, Mar 13, 2019 at 11:17 AM Sergei Kornilov <sk@zsrv.org> wrote: > > The buildfarm thinks additional nitpicking is needed. > > hm. Patch was committed with debug1 level tests and many animals uses log_statement = 'all'. Therefore they have additionalline in result: LOG: statement: alter table pg_class alter column relname drop not null; and similar for otherqueries. > I think we better would be to revert "set client_min_messages to 'debug1';" part. Ugh, I guess so. Or how about changing the message itself to use INFO, like we already do in QueuePartitionConstraintValidation? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi > Ugh, I guess so. Or how about changing the message itself to use > INFO, like we already do in QueuePartitionConstraintValidation? Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us regards, Sergei
Sergei Kornilov <sk@zsrv.org> writes: >> Ugh, I guess so. Or how about changing the message itself to use >> INFO, like we already do in QueuePartitionConstraintValidation? > Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us What I thought then was that you didn't need the message at all, at any debug level. I still think that. It might have been useful for development purposes but it does not belong in committed code. INFO (making it impossible for anybody to not have the message in-their-face) is right out. regards, tom lane
I wrote: > Sergei Kornilov <sk@zsrv.org> writes: >>> Ugh, I guess so. Or how about changing the message itself to use >>> INFO, like we already do in QueuePartitionConstraintValidation? >> Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us > What I thought then was that you didn't need the message at all, > at any debug level. I still think that. Oh, and yes, I think QueuePartitionConstraintValidation's usage is an unacceptable abuse of INFO level. I'm surprised we haven't gotten complaints about it yet. regards, tom lane
On Wed, Mar 13, 2019 at 1:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Sergei Kornilov <sk@zsrv.org> writes: > >> Ugh, I guess so. Or how about changing the message itself to use > >> INFO, like we already do in QueuePartitionConstraintValidation? > > > Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us > > What I thought then was that you didn't need the message at all, > at any debug level. I still think that. It might have been useful > for development purposes but it does not belong in committed code. > INFO (making it impossible for anybody to not have the message > in-their-face) is right out. I find that position entirely wrong-headed. If you think users have no interest in a message telling them whether their gigantic table is getting scanned or not, you're wrong. Maybe you're right that everyone doesn't want it, but I'm positive some do. We've had requests for very similar things on this very mailing list more than one. And if you think that it's not useful to have regression tests that verify that the behavior is correct, well, that's not a question with one objectively right answer, but I emphatically disagree with that position. This behavior is often quite subtle, especially in combination with partitioned tables where different partitions may get different treatment. It would not be at all difficult to break it without realizing. I do understand that we seem to have no good way of doing this that lets users have the option of seeing this message and also makes it something that can be regression-tested. INFO is out because there's no option, and DEBUG1 is out because it doesn't work in the regression tests. However, I think giving up and saying "ok, well that's just how it is, too bad" is, one, letting the tail wag the dog, and two, a terribly disappointing attitude. I've just reverted the 0002 portion of the patch, which should hopefully fix this problem for now. But I strongly encourage you think of something to which you could say "yes" instead of just shooting everything people want to do in this area down. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Mar 13, 2019 at 1:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Oh, and yes, I think QueuePartitionConstraintValidation's usage > is an unacceptable abuse of INFO level. I'm surprised we haven't > gotten complaints about it yet. Perhaps that's because users aren't as direly opposed to informational messages from DDL commands as you seem to believe... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Hi, On Wed, Mar 13, 2019 at 01:25:11PM -0400, Robert Haas wrote: > On Wed, Mar 13, 2019 at 1:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Oh, and yes, I think QueuePartitionConstraintValidation's usage > > is an unacceptable abuse of INFO level. I'm surprised we haven't > > gotten complaints about it yet. > > Perhaps that's because users aren't as direly opposed to informational > messages from DDL commands as you seem to believe... On Wed, Mar 13, 2019 at 01:09:32PM -0400, Tom Lane wrote: > Sergei Kornilov <sk@zsrv.org> writes: > >> Ugh, I guess so. Or how about changing the message itself to use > >> INFO, like we already do in QueuePartitionConstraintValidation? > > > Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose: https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us > > What I thought then was that you didn't need the message at all, > at any debug level. I still think that. It might have been useful > for development purposes but it does not belong in committed code. > INFO (making it impossible for anybody to not have the message > in-their-face) is right out. I'm writing to provide a datapoint for the existing message regarding "implied by existing constraints" in QueuePartitionConstraintValidation. On the one hand, I agree that message at INFO is more verbose than other commands, and I'm 20% surprised by it. On the other hand, I *like* the output (including at INFO level). It alerted me to 1) deficiency with some of our tables (due to column not marked NOT NULL); and, 2) helped me to understand how to improve some of our dynamic DDL. If the message weren't visible at LOG, I'd miss it, at least sometimes, and look for how to re-enable it (but statement logging of DEBUG is beyond excessive). For context, I'm someone who configures servers with log_min_messages=info and who sometimes SETs client_min_messages='DEBUG1' ; I have $toomany warnings in our monitoring system, because I'd rather see twice as much stuff that may or may not be interesting than miss 10% of things I needed to see. Justin
On Thu, 14 Mar 2019 at 06:24, Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Mar 13, 2019 at 1:09 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Sergei Kornilov <sk@zsrv.org> writes: > > >> Ugh, I guess so. Or how about changing the message itself to use > > >> INFO, like we already do in QueuePartitionConstraintValidation? > > > > > Fine for me. But year ago this was implemented in my patch and Tom voted against using INFO level for such purpose:https://www.postgresql.org/message-id/1142.1520362313%40sss.pgh.pa.us > > > > What I thought then was that you didn't need the message at all, > > at any debug level. I still think that. It might have been useful > > for development purposes but it does not belong in committed code. > > INFO (making it impossible for anybody to not have the message > > in-their-face) is right out. > > I find that position entirely wrong-headed. If you think users have > no interest in a message telling them whether their gigantic table is > getting scanned or not, you're wrong. Maybe you're right that > everyone doesn't want it, but I'm positive some do. We've had > requests for very similar things on this very mailing list more than > one. If we don't have this for SET NOT NULL then we should either add it or remove the one from ATTACH PARTITION. I don't think we need to decide which it is now, so I've added an item to the open items list that this is out for debate. https://wiki.postgresql.org/wiki/PostgreSQL_12_Open_Items#Open_Issues I think we can mark this patch as committed now as I don't think the lack of a way to test it is likely to cause it to be reverted. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Hello > If we don't have this for SET NOT NULL then we should either add it or > remove the one from ATTACH PARTITION. I proposed to lower report level to debug1 in a separate thread: https://www.postgresql.org/message-id/4859321552643736%40myt5-02b80404fd9e.qloud-c.yandex.net Possible better would be link it. regards, Sergei
Hi Robert, On 3/17/19 3:40 PM, David Rowley wrote: > On Thu, 14 Mar 2019 at 06:24, Robert Haas <robertmhaas@gmail.com> wrote: > > I think we can mark this patch as committed now as I don't think the > lack of a way to test it is likely to cause it to be reverted. Should we mark this as committed now or is there more tweaking to be done? Regards, -- -David david@pgmasters.net
On Mon, Mar 25, 2019 at 3:51 AM David Steele <david@pgmasters.net> wrote: > On 3/17/19 3:40 PM, David Rowley wrote: > > On Thu, 14 Mar 2019 at 06:24, Robert Haas <robertmhaas@gmail.com> wrote: > > > > I think we can mark this patch as committed now as I don't think the > > lack of a way to test it is likely to cause it to be reverted. > > Should we mark this as committed now or is there more tweaking to be done? I have now marked it as Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 2019-03-25 11:09:47 -0400, Robert Haas wrote: > On Mon, Mar 25, 2019 at 3:51 AM David Steele <david@pgmasters.net> wrote: > > On 3/17/19 3:40 PM, David Rowley wrote: > > > On Thu, 14 Mar 2019 at 06:24, Robert Haas <robertmhaas@gmail.com> wrote: > > > > > > I think we can mark this patch as committed now as I don't think the > > > lack of a way to test it is likely to cause it to be reverted. > > > > Should we mark this as committed now or is there more tweaking to be done? > > I have now marked it as Committed. There's still an open item "Debate INFO messages in ATTACH PARTITION and SET NOT NULL" for this thread. Is there anything further to be done? Or are we content with this for v12? - Andres
Andres Freund <andres@anarazel.de> writes:
> There's still an open item "Debate INFO messages in ATTACH PARTITION and
> SET NOT NULL" for this thread. Is there anything further to be done? Or
> are we content with this for v12?
IIRC, that's based on my complaint that these messages have no business
being INFO level. I'm definitely not content with the current state.
I proposed that we didn't need those messages at all, which Robert pushed
back on, but I'd be willing to compromise by reducing them to NOTICE or
DEBUG level.
The actually intended use of INFO level is to mark messages that the user
actively asked for (via VERBOSE or some morally-equivalent option), which
is why that level has such high priority. If we had something like a
VERBOSE option for ALTER TABLE, it'd make plenty of sense to follow
VACUUM's precedent:
if (params->options & VACOPT_VERBOSE)
elevel = INFO;
else
elevel = DEBUG2;
It seems a bit late to be inventing such a thing for v12 though.
In the meantime, I'm not very content with random subsets of
ALTER TABLE acting as if they have been granted permission
to be VERBOSE. It's unfriendly, and it's inconsistent because
there are so many other expensive ALTER TABLE actions that don't
do this.
regards, tom lane
Hi > I proposed that we didn't need those messages at all, which Robert pushed > back on, but I'd be willing to compromise by reducing them to NOTICE or > DEBUG level. I posted patch with such change in a separate topic: https://commitfest.postgresql.org/23/2076/ PS: not think this is blocker for v12 regards, Sergei
On Thu, 2 May 2019 at 06:18, Sergei Kornilov <sk@zsrv.org> wrote: > > > I proposed that we didn't need those messages at all, which Robert pushed > > back on, but I'd be willing to compromise by reducing them to NOTICE or > > DEBUG level. > > I posted patch with such change in a separate topic: https://commitfest.postgresql.org/23/2076/ > > PS: not think this is blocker for v12 Probably not a blocker, but now does not seem like an unreasonable time to lower these INFO messages down to DEBUG. If not, then we can just remove the item from the open items list. I just put it there as I didn't want it getting forgotten about as it wasn't going to be anybody's priority to think hard about it on the final few days of the last commitfest of the release. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
David Rowley <david.rowley@2ndquadrant.com> writes:
> On Thu, 2 May 2019 at 06:18, Sergei Kornilov <sk@zsrv.org> wrote:
>> PS: not think this is blocker for v12
> Probably not a blocker, but now does not seem like an unreasonable
> time to lower these INFO messages down to DEBUG.
Not a blocker perhaps, but it's better if we can get new behavior to
be more or less right the first time.
regards, tom lane
On Thu, 2 May 2019 at 13:08, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > David Rowley <david.rowley@2ndquadrant.com> writes: > > On Thu, 2 May 2019 at 06:18, Sergei Kornilov <sk@zsrv.org> wrote: > >> PS: not think this is blocker for v12 > > > Probably not a blocker, but now does not seem like an unreasonable > > time to lower these INFO messages down to DEBUG. > > Not a blocker perhaps, but it's better if we can get new behavior to > be more or less right the first time. It's not really new behaviour though. The code in question is for ATTACH PARTITION and was added in c31e9d4bafd (back in 2017). bbb96c3704c is the commit for using constraints to speed up SET NOT NULL. The mention of using the constraint for proof was made DEBUG1 in the initial commit. What we need to decide is if we want to make ATTACH PARTITION's INFO message a DEBUG1 in pg12 or not. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Thu, 2 May 2019 at 14:22, David Rowley <david.rowley@2ndquadrant.com> wrote: > > On Thu, 2 May 2019 at 13:08, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Not a blocker perhaps, but it's better if we can get new behavior to > > be more or less right the first time. > > It's not really new behaviour though. The code in question is for > ATTACH PARTITION and was added in c31e9d4bafd (back in 2017). > bbb96c3704c is the commit for using constraints to speed up SET NOT > NULL. The mention of using the constraint for proof was made DEBUG1 in > the initial commit. What we need to decide is if we want to make > ATTACH PARTITION's INFO message a DEBUG1 in pg12 or not. FWIW, I'm in favour of changing ATTACH PARTITION's INFO messages to DEBUG1 for PG12 to align it to what bbb96c3704c did. Anyone else? -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Fri, 3 May 2019 at 19:06, David Rowley <david.rowley@2ndquadrant.com> wrote: > FWIW, I'm in favour of changing ATTACH PARTITION's INFO messages to > DEBUG1 for PG12 to align it to what bbb96c3704c did. > > Anyone else? Does anyone think we shouldn't change the INFO message in ATTACH PARTITION to a DEBUG1 in PG12? I think it makes sense make this change so that it matches the behaviour of the output of ALTER TABLE .. ALTER COLUMN .. SET NOT NULL when it uses a CHECK constraint. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Hello > Does anyone think we shouldn't change the INFO message in ATTACH > PARTITION to a DEBUG1 in PG12? Seems no one wants to vote against this change. My proposed patch was here: https://commitfest.postgresql.org/23/2076/ regards, Sergei
On Tue, 11 Jun 2019 at 03:35, Sergei Kornilov <sk@zsrv.org> wrote: > > Does anyone think we shouldn't change the INFO message in ATTACH > > PARTITION to a DEBUG1 in PG12? > > Seems no one wants to vote against this change. I'm concerned about two things: 1. The patch reduces the test coverage of ATTACH PARTITION. We now have no way to ensure the constraint was used to validate the rows in the partition. 2. We're inconsistent with what we do for SET NOT NULL and ATTACH PARTITION. We raise an INFO message when we use a constraint for ATTACH PARTITION and only a DEBUG1 for SET NOT NULL. Unfortunately, my two concerns conflict with each other. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Jun 12, 2019 at 08:34:57AM +1200, David Rowley wrote: >On Tue, 11 Jun 2019 at 03:35, Sergei Kornilov <sk@zsrv.org> wrote: >> > Does anyone think we shouldn't change the INFO message in ATTACH >> > PARTITION to a DEBUG1 in PG12? >> >> Seems no one wants to vote against this change. > >I'm concerned about two things: > >1. The patch reduces the test coverage of ATTACH PARTITION. We now >have no way to ensure the constraint was used to validate the rows in >the partition. >2. We're inconsistent with what we do for SET NOT NULL and ATTACH >PARTITION. We raise an INFO message when we use a constraint for >ATTACH PARTITION and only a DEBUG1 for SET NOT NULL. > >Unfortunately, my two concerns conflict with each other. > We're getting close to beta3/rc1, and this thread was idle for ~1 month. I think there's a consensus to change INFO to DEBUG1 in pg12, and then maybe imlpement something like VERBOSE mode in the future. Objections? As for the reduction of test coverage, can't we deduce whether a constraint was used from data in pg_stats or something like that? regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:
> I think there's a consensus to change INFO to DEBUG1 in pg12, and then
> maybe imlpement something like VERBOSE mode in the future. Objections?
ISTM the consensus is "we'd rather reduce the verbosity, but we don't
want to give up test coverage". So what's blocking this is lack of
a patch to show that there's another way to verify what code path
was taken.
> As for the reduction of test coverage, can't we deduce whether a
> constraint was used from data in pg_stats or something like that?
Not sure how exactly ... and we've already learned that pg_stats
isn't too reliable.
regards, tom lane