Re: using index or check in ALTER TABLE SET NOT NULL

Tomas Vondra 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

Re: using index or check in ALTER TABLE SET NOT NULL

On Wed, Jun 12, 2019 at 08:34:57AM +1200, David Rowley wrote: On Tue, 11 Jun 2019 at 03:35, Sergei Kornilov 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

Re: using index or check in ALTER TABLE SET NOT NULL

On Tue, 11 Jun 2019 at 03:35, Sergei Kornilov 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

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

On Fri, 3 May 2019 at 19:06, David Rowley 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

Re: using index or check in ALTER TABLE SET NOT NULL

On Thu, 2 May 2019 at 14:22, David Rowley wrote: > > On Thu, 2 May 2019 at 13:08, Tom Lane 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

Re: using index or check in ALTER TABLE SET NOT NULL

On Thu, 2 May 2019 at 13:08, Tom Lane wrote: > > David Rowley writes: > > On Thu, 2 May 2019 at 06:18, Sergei Kornilov 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.

Re: using index or check in ALTER TABLE SET NOT NULL

David Rowley writes: > On Thu, 2 May 2019 at 06:18, Sergei Kornilov 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

Re: using index or check in ALTER TABLE SET NOT NULL

On Thu, 2 May 2019 at 06:18, Sergei Kornilov 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: >

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

Andres Freund 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

Re: using index or check in ALTER TABLE SET NOT NULL

On 2019-03-25 11:09:47 -0400, Robert Haas wrote: > On Mon, Mar 25, 2019 at 3:51 AM David Steele wrote: > > On 3/17/19 3:40 PM, David Rowley wrote: > > > On Thu, 14 Mar 2019 at 06:24, Robert Haas wrote: > > > > > > I think we can mark this patch as committed now as I don't think the > > > lack of

Re: Re: using index or check in ALTER TABLE SET NOT NULL

On Mon, Mar 25, 2019 at 3:51 AM David Steele wrote: > On 3/17/19 3:40 PM, David Rowley wrote: > > On Thu, 14 Mar 2019 at 06:24, Robert Haas 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. > >

Re: Re: using index or check in ALTER TABLE SET NOT NULL

Hi Robert, On 3/17/19 3:40 PM, David Rowley wrote: On Thu, 14 Mar 2019 at 06:24, Robert Haas 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

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

On Thu, 14 Mar 2019 at 06:24, Robert Haas wrote: > > On Wed, Mar 13, 2019 at 1:09 PM Tom Lane wrote: > > Sergei Kornilov writes: > > >> Ugh, I guess so. Or how about changing the message itself to use > > >> INFO, like we already do in QueuePartitionConstraintValidation? > > > > > Fine for me.

Re: using index or check in ALTER TABLE SET NOT NULL

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 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. > >

Re: using index or check in ALTER TABLE SET NOT NULL

On Wed, Mar 13, 2019 at 1:19 PM Tom Lane 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

Re: using index or check in ALTER TABLE SET NOT NULL

On Wed, Mar 13, 2019 at 1:09 PM Tom Lane wrote: > Sergei Kornilov 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 > >

Re: using index or check in ALTER TABLE SET NOT NULL

I wrote: > Sergei Kornilov 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:

Re: using index or check in ALTER TABLE SET NOT NULL

Sergei Kornilov 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: >

Re: using index or check in ALTER TABLE SET NOT NULL

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:

Re: using index or check in ALTER TABLE SET NOT NULL

On Wed, Mar 13, 2019 at 11:17 AM Sergei Kornilov 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 additional line in result: LOG: > statement: alter table

Re: using index or check in ALTER TABLE SET NOT NULL

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 additional line in result: LOG: statement: alter table pg_class alter column relname drop not null; and similar for

Re: using index or check in ALTER TABLE SET NOT NULL

Robert Haas writes: > On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov wrote: >> Agreed, in attached new version ... > Committed with a little more nitpicking. The buildfarm thinks additional nitpicking is needed. regards, tom lane

Re: using index or check in ALTER TABLE SET NOT NULL

Wow, thank you! 13.03.2019, 15:57, "Robert Haas" : > On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov wrote: >>  Agreed, in attached new version ... > > Committed with a little more nitpicking. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company

Re: using index or check in ALTER TABLE SET NOT NULL

On Tue, Mar 12, 2019 at 4:27 PM Sergei Kornilov wrote: > Agreed, in attached new version ... Committed with a little more nitpicking. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company

Re: using index or check in ALTER TABLE SET NOT NULL

Hello > Dispatches from the department of grammatical nitpicking... Thank you! > + entire table, however if a valid CHECK 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

Re: using index or check in ALTER TABLE SET NOT NULL

On Sun, Mar 10, 2019 at 11:30 PM David Rowley 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

Re: using index or check in ALTER TABLE SET NOT NULL

On Mon, 11 Mar 2019 at 00:13, Sergei Kornilov 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

Re: using index or check in ALTER TABLE SET NOT NULL

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, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 890b23afd6..926b3361ea 100644 ---

Re: using index or check in ALTER TABLE SET NOT NULL

On Wed, 6 Mar 2019 at 08:41, Sergei Kornilov 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 >

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

On Sun, 15 Apr 2018 at 19:09, Sergei Kornilov 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

Re: using index or check in ALTER TABLE SET NOT NULL

> On Thu, Nov 22, 2018 at 6:04 PM Dmitry Dolgov <9erthali...@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

Re: using index or check in ALTER TABLE SET NOT NULL

> On Tue, Nov 13, 2018 at 1:59 PM Alvaro Herrera > wrote: > > On 2018-Nov-13, Dmitry Dolgov wrote: > > > > On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov wrote: > > > > > > > If not properly cataloguing NOT NULL constraints would be fixed, can it > > > > potentially conflict with the current

Re: using index or check in ALTER TABLE SET NOT NULL

On 2018-Nov-13, Dmitry Dolgov wrote: > > On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov 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

Re: using index or check in ALTER TABLE SET NOT NULL

> On Sun, 4 Nov 2018 at 19:03, Sergei Kornilov 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

Re: using index or check in ALTER TABLE SET NOT NULL

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 CF was no further activity and patch was reverted back to "need review" with

Re: using index or check in ALTER TABLE SET NOT NULL

>On Sun, 15 Apr 2018 at 09:09, Sergei Kornilov 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.

Re: using index or check in ALTER TABLE SET NOT NULL

Hello Attached updated patch follows recent Reorganize partitioning code commit. regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index bd22627..db98a98 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -215,8

Re: using index or check in ALTER TABLE SET NOT NULL

Hi I noticed new merge conflict, updated version attached. regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index bd22627..db98a98 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -215,8 +215,15 @@ WITH (

Re: using index or check in ALTER TABLE SET NOT NULL

Hello I notice my patch does not apply again. Here is update to current HEAD regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index bd22627..db98a98 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -215,8

Re: using index or check in ALTER TABLE SET NOT NULL

Hello, Peter! I agree my patch seems as strange workaround. And better will be SET NOT NULL NOT VALID feature. But this change is very complicated for me For now my patch is small, simple and provides way for users. regards, Sergei 06.04.2018, 06:29, "Peter Eisentraut"

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

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 may be broken in future without test fail. alter table will

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

Sergei Kornilov 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 decided that this

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

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 level instead DEBUG1, similarly > ATTACH PARTITION code. Updated patch attached. > > Or i can rewrite tests to use DEBUG1 level. You

Re: using index or check in ALTER TABLE SET NOT NULL

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 level instead DEBUG1, similarly ATTACH PARTITION code. Updated patch attached. Or i can rewrite tests to use DEBUG1 level. regards, Sergeidiff --git

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

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

Re: using index or check in ALTER TABLE SET NOT NULL

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 numeric_literal, REM + Full table scan is performed to check that no existing row + in the table has null values in given column.

Re: using index or check in ALTER TABLE SET NOT NULL

Hello Stephen I changed the suggested things and added some regression tests. Also i wrote few words to the documentation. New patch is attached. Thank you for feedback! regards, Sergeidiff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 286c7a8..cf2c56f

Re: using index or check in ALTER TABLE SET NOT NULL

Greetings Sergei, * Sergei Kornilov (s...@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

Re: using index or check in ALTER TABLE SET NOT NULL

Hello I update patch and also rebase to current head. I hope now it is better aligned with project stylediff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c index dd4a8d3..36bcb3f 100644 --- a/src/backend/catalog/partition.c +++ b/src/backend/catalog/partition.c @@

Re: using index or check in ALTER TABLE SET NOT NULL

Thank you for review! My apologies for my errors. It seems i read developers wiki pages not enough carefully. I will reread wiki, code style and then update patch with all your remarks. > The comment /* T if we added new NOT NULL constraints */ should > probably be changed to /* T if we should

Re: using index or check in ALTER TABLE SET NOT NULL

On November 29, 2017 8:50:31 AM PST, Stephen Frost 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 could

Re: using index or check in ALTER TABLE SET NOT NULL

Here is new patch with check only existed valid constraints and without SPI at all. Thanksdiff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index d979ce2..7ab7580 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -370,6 +370,8

Re: using index or check in ALTER TABLE SET NOT NULL

Stephen Frost writes: > * Tom Lane (t...@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

Re: using index or check in ALTER TABLE SET NOT NULL

On Wed, Nov 29, 2017 at 10:52 AM, Sergei Kornilov wrote: > I agree this. Thinking a little about idea of index scan i can not give > reasonable usecase which required index. My target problem of adding NOT NULL > to big relation without long downtime can be done with ADD

Re: using index or check in ALTER TABLE SET NOT NULL

Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost 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

Re: using index or check in ALTER TABLE SET NOT NULL

Robert, * Robert Haas (robertmh...@gmail.com) wrote: > On Tue, Nov 28, 2017 at 1:59 PM, Sergei Kornilov wrote: > > I write patch to speed up ALTER TABLE SET NOT NULL by check existed check > > constraints or indexes. Huge phase 3 with verify table data will be skipped > > if

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 verify table data will be skipped if table has valid check constraint cover "alteredfield IS NOT NULL" condition or by SPI query if found index with compatible condition or