Change ereport level for QueuePartitionConstraintValidation

On Fri, Mar 15, 2019 at 12:55:36PM +0300, Sergei Kornilov wrote:

We have INFO ereport messages in alter table attach partition like this:

partition constraint for table \"%s\" is implied by existing constraints

So now I am +1 to idea of change error level for this messages. I attach patch to lower such ereport to DEBUG1 level

+1

I reviewed existing logging behavior and now I agree.

Also, I wondered if it was worth considering a way to configure logging which
scales better than boolean GUCs:

log_duration
log_checkpoints
log_(dis)connections
log_lock_waits
log_replication_commands
..plus a bunch more developer ones:
https://www.postgresql.org/docs/current/runtime-config-developer.html

I'm (very tentatively) thinking of a string GUC which is split on whitespace
and is parsed into a bitmap which is consulted instead of the existing vars, as
in: if (logging_bits & LOG_CHECKPOINTS) ... which could be either an enum or
#define.. If there's an entry in logging_bits which isn't recognized, I guess
it'd be logged at NOTICE or WARNING.

I'd also request this be conditional promoted from DEBUG1 to LOG depending a
new logging_bit for LOG_PARALLEL_WORKER:
|starting background worker process "parallel worker for PID...
When written to csvlog (and when using log_min_error_severity=notice or
similar), that would help answering questions like: "what queries are my
max_parallel_workers(_per_process) being used for (at the possible exclusion of
other queries)?". I noticed on our servers that a query running possibly every
~10sec had been using parallel query, which not only hurt that query, but also
meant that works may have been unavailable for report queries which could have
benefited from their use. It'd be nice to know if there were other such issues.

Justin

Hello

This change is discussed as open item for pg12. Seems we have nor objections nor agreement. I attached updated version due merge conflict.

Per discussion started here: /messages/by-id/CA+TgmoZWSLUjVcc9KBSVvbn=U5QRgW1O-MgUX0y5CnLZOA2qyQ@mail.gmail.com

regards, Sergei

On Tue, Jul 2, 2019 at 12:17 AM Sergei Kornilov <sk@zsrv.org> wrote:

This change is discussed as open item for pg12. Seems we have nor objections nor agreement. I attached updated version due merge conflict.

Per discussion started here: /messages/by-id/CA+TgmoZWSLUjVcc9KBSVvbn=U5QRgW1O-MgUX0y5CnLZOA2qyQ@mail.gmail.com

I took the liberty of setting this to "Ready for Committer" to see if
we can get a decision one way or another and clear both a Commitfest
item and a PG12 Open Item. No committer is signed up, but it looks
like Amit L wrote the messages in question, Robert committed them, and
David made arguments for AND against on the referenced thread, so I'm
CCing them, and retreating to a safe distance.

--
Thomas Munro
https://enterprisedb.com

On Mon, 15 Jul 2019 at 11:46, Thomas Munro <thomas.munro@gmail.com> wrote:

On Tue, Jul 2, 2019 at 12:17 AM Sergei Kornilov <sk@zsrv.org> wrote:

This change is discussed as open item for pg12. Seems we have nor objections nor agreement. I attached updated version due merge conflict.

Per discussion started here: /messages/by-id/CA+TgmoZWSLUjVcc9KBSVvbn=U5QRgW1O-MgUX0y5CnLZOA2qyQ@mail.gmail.com

I took the liberty of setting this to "Ready for Committer" to see if
we can get a decision one way or another and clear both a Commitfest
item and a PG12 Open Item. No committer is signed up, but it looks
like Amit L wrote the messages in question, Robert committed them, and
David made arguments for AND against on the referenced thread, so I'm
CCing them, and retreating to a safe distance.

I think the only argument against it was around lack of ability to
test if the constraint was used to verify no row breaks the partition
bound during the ATTACH PARTITION.

Does anyone feel strongly that we need to the test to confirm that the
constraint was used for this?

If nobody feels so strongly about that then I say we can just push
this. It seems something that's unlikely to get broken, but then you
could probably say that for most things our tests test for.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Sun, Jul 14, 2019 at 7:46 PM Thomas Munro <thomas.munro@gmail.com> wrote:

... and retreating to a safe distance.

Is that measure in, like, light-years?

I vote for changing it to NOTICE instead of DEBUG1.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2019-Jul-15, David Rowley wrote:

I think the only argument against it was around lack of ability to
test if the constraint was used to verify no row breaks the partition
bound during the ATTACH PARTITION.

Would it work to set client_min_messages to DEBUG1 for the duration of
the test, or does that have too much unrelated noise?

Does anyone feel strongly that we need to the test to confirm that the
constraint was used for this?

Well, IME if we don't test it, we're sure to break it in the future.
The only questions are 1) when, 2) how long till we notice, 3) how
difficult is it to fix at that point. I think breakage is easily
noticed by users, and a fix is unlikely to require hard measures such as
ABI breaks or catversion bumps. I'd like more than zero tests, but it
doesn't seem *that* severe.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, 16 Jul 2019 at 03:13, Robert Haas <robertmhaas@gmail.com> wrote:

I vote for changing it to NOTICE instead of DEBUG1.

Well, there are certainly other DDL commands that spit out NOTICES.

postgres=# create table z (a int);
CREATE TABLE
postgres=# create table x (a int) inherits(z);
NOTICE: merging column "a" with inherited definition
CREATE TABLE

However, we did get rid of a few of those a while back. In 9.2 we used to have:

postgres=# create table a (a int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"a_pkey" for table "a"

I'm pretty keen for consistency. Having ATTACH PARTITION spit out an
INFO and merge attributes a NOTICE, and SET NOT NULL just a DEBUG1 is
pretty far from consistent. I wouldn't object to making them all
NOTICE. I've only seen complaints about the INFO one.

Would anyone complain if we made them all INFO?

If we do that should we backpatch the change into PG12. SET NOT NULL
using a constraint was new there.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

David Rowley <david.rowley@2ndquadrant.com> writes:

Would anyone complain if we made them all INFO?

That would be remarkably horrid, because that makes them unsuppressable.

I'm generally for having these be less in-your-face, not more so.

regards, tom lane

On Tue, Jul 16, 2019 at 10:15 AM David Rowley
<david.rowley@2ndquadrant.com> wrote:

On Tue, 16 Jul 2019 at 03:13, Robert Haas <robertmhaas@gmail.com> wrote:

I vote for changing it to NOTICE instead of DEBUG1.

Well, there are certainly other DDL commands that spit out NOTICES.

postgres=# create table z (a int);
CREATE TABLE
postgres=# create table x (a int) inherits(z);
NOTICE: merging column "a" with inherited definition
CREATE TABLE

However, we did get rid of a few of those a while back. In 9.2 we used to have:

postgres=# create table a (a int primary key);
NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index
"a_pkey" for table "a"

I'm pretty keen for consistency. Having ATTACH PARTITION spit out an
INFO and merge attributes a NOTICE, and SET NOT NULL just a DEBUG1 is
pretty far from consistent. I wouldn't object to making them all
NOTICE. I've only seen complaints about the INFO one.

Fwiw, I'm leaning toward NOTICE for all. It's helpful for users to
know a certain action was taken.

Thanks,
Amit

Hello

Here is two patches with NOTICE ereport: one for partitions operations and one for "set not null" (for consistency)

regards, Sergei

On 2019-Jul-15, David Rowley wrote:

I think the only argument against it was around lack of ability to
test if the constraint was used to verify no row breaks the partition
bound during the ATTACH PARTITION.

Would it work to set client_min_messages to DEBUG1 for the duration of
the test, or does that have too much unrelated noise?

Does anyone feel strongly that we need to the test to confirm that the
constraint was used for this?

Well, IME if we don't test it, we're sure to break it in the future.
The only questions are 1) when, 2) how long till we notice, 3) how
difficult is it to fix at that point. I think breakage is easily
noticed by users, and a fix is unlikely to require hard measures such as
ABI breaks or catversion bumps. I'd like more than zero tests, but it
doesn't seem *that* severe.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2019-Jul-15, David Rowley wrote:

I think the only argument against it was around lack of ability to
test if the constraint was used to verify no row breaks the partition
bound during the ATTACH PARTITION.

Would it work to set client_min_messages to DEBUG1 for the duration of
the test, or does that have too much unrelated noise?

It's not awful. I tried inserting "set client_min_messages = debug1"
into alter_table.sql, and got the attached diffs. Evidently we
could not keep it on throughout that test script, because of the
variable OIDs in some of the toast table names. But in the areas
where we're currently emitting INFO messages, we could have it on
and not have any other noise except some "verifying table" messages,
which actually seem like a good thing for this test.

So right at the moment my vote is to downgrade all of these to DEBUG1
and fix the test-coverage complaint by adjusting client_min_messages
as needed in the test scripts.

A potential objection is that this'd constrain people's ability to add
DEBUG1 messages in code reachable from ALTER TABLE --- but we can
cross that bridge when we come to it.

regards, tom lane

Hi

It's not awful. I tried inserting "set client_min_messages = debug1"
into alter_table.sql

We already did this in March. And this change was reverted in 5655565c077c53b6e9b4b9bfcdf96439cf3af065 because this will not work on buildfarm animals with log_statement = 'all'

regards, Sergei

Sergei Kornilov <sk@zsrv.org> writes:

It's not awful. I tried inserting "set client_min_messages = debug1"
into alter_table.sql

We already did this in March. And this change was reverted in 5655565c077c53b6e9b4b9bfcdf96439cf3af065 because this will not work on buildfarm animals with log_statement = 'all'

Oh :-(.

Seems like maybe what we need is to transpose the tests at issue into
a TAP test? That could grep for the messages we care about and disregard
other ones.

regards, tom lane

On Thu, 18 Jul 2019 at 07:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Seems like maybe what we need is to transpose the tests at issue into
a TAP test? That could grep for the messages we care about and disregard
other ones.

That seems like a good idea. I guess that's a vote in favour of
having DEBUG1 for ATTACH PARTITION and SET NOT NULL too?

I don't know my way around the tap tests that well, but I started to
look at this and ended up a bit stuck on where the test should be
located. I see src/test/modules/brin has some brin related tests, so
I thought that src/test/modules/alter_table might be the spot, but
after looking at src/test/README I see it mentions that only tests
that are themselves an extension should be located within:

modules/
Extensions used only or mainly for test purposes, generally not suitable
for installing in production databases

There are a few others in the same situation as brin; commit_ts,
snapshot_too_old, unsafe_tests. I see unsafe_tests does mention the
lack of module in the README file.

Is there a better place to do the alter_table ones? Or are the above
ones in there because there's no better place?

Also, if I'm not wrong, the votes so far appear to be:

NOTICE: Robert, Amit
DEBUG1: Tom, Alvaro (I'm entirely basing this on the fact that they
mentioned possible ways to test with DEBUG1)

I'll be happy with DEBUG1 if we can get tests to test it.

--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 2019-Jul-23, David Rowley wrote:

Also, if I'm not wrong, the votes so far appear to be:

NOTICE: Robert, Amit
DEBUG1: Tom, Alvaro (I'm entirely basing this on the fact that they
mentioned possible ways to test with DEBUG1)

I'll be happy with DEBUG1 if we can get tests to test it.

Well, I think the user doesn't *care* to see a message about the
optimization. They just want the command to be fast. *We* (developers)
want the message in order to ensure the command remains fast. So some
DEBUG level seems the right thing.

Another way to reach the same conclusion is to think about the "building
index ... serially" messages, which are are pretty much in the same
category and are using DEBUG1. (I do think the TOAST ones are just
noise though, and since they disrupt potential testing with
client_min_messages=debug1, another way to go about this is to reduce
those to DEBUG2 or just elide them.)

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2019-Jul-23, David Rowley wrote:

I don't know my way around the tap tests that well, but I started to
look at this and ended up a bit stuck on where the test should be
located. I see src/test/modules/brin has some brin related tests, so
I thought that src/test/modules/alter_table might be the spot, but
after looking at src/test/README I see it mentions that only tests
that are themselves an extension should be located within:

modules/
Extensions used only or mainly for test purposes, generally not suitable
for installing in production databases

There are a few others in the same situation as brin; commit_ts,
snapshot_too_old, unsafe_tests. I see unsafe_tests does mention the
lack of module in the README file.

The readme in src/test/modules says "extensions or libraries", and I see
no reason to think that a TAP test would be totally out of place there.
I think the alter_table/ subdir is a perfect place.

Sergei, can we enlist you to submit a patch for this? Namely reduce the
log level to DEBUG1 and add a TAP test in src/test/modules/alter_table/
that verifies that the message is or isn't emitted, as appropriate.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hi

Sergei, can we enlist you to submit a patch for this? Namely reduce the
log level to DEBUG1 and add a TAP test in src/test/modules/alter_table/
that verifies that the message is or isn't emitted, as appropriate.

Yes, will do. Probably in few days.

regards, Sergei

Hello

Sergei, can we enlist you to submit a patch for this? Namely reduce the
log level to DEBUG1 and add a TAP test in src/test/modules/alter_table/
that verifies that the message is or isn't emitted, as appropriate.

I created this patch.
I test message existence. Also I check message "verifying table" (generated on DEBUG1 from ATRewriteTable). So with manually damaged logic in NotNullImpliedByRelConstraints or ConstraintImpliedByRelConstraint "make check" may works but fails on new test during "make check-world". As we want.

regards, Sergei