date:20210622

RE: Partition Check not updated when insert into a partition

2021-06-22 Thread houzj.f...@fujitsu.com

On Wednesday, June 23, 2021 12:16 PM I wrote:
> When directly INSERT INTO partition, postgres will invoke ExecPartitionCheck
> which will execute its parent's and grandparent's partition constraint check.
> From the code, the whole constraint check is saved in relcache::rd_partcheck.
> 
> For a multi-level partition, for example: table 'A' is partition of table 
> 'B', and 'B'
> is also partition of table 'C'. After I 'ALTER TABLE C DETACH B', I thought
> partition constraint check of table 'C' does not matter anymore if INSERT INTO
> table 'A'. But it looks like the relcache of 'A' is not invalidated after 
> detaching 'B'.
> And the relcache::rd_partcheck still include the partition constraint of 
> table 'C'.
> Note If I invalidate the table 'A''s relcache manually, then next time the
> relcache::rd_partcheck will be updated to the expected one which does not
> include partition constraint check of table 'C'.
> (ATTACH partition has the same behaviour that relcache::rd_partcheck will not
> be updated immediately)

An DETACH PARTITION example which shows the relcache::rd_partcheck
is not invalidated immediately is:

- parttable1 -> parttable2-> parttable3
create table parttable1 (a int, b int, c int) partition by list(a);
create table parttable2 (a int, b int, c int) partition by list(b);
create table parttable3 (a int, b int, c int);
alter table parttable1 attach partition parttable2 for values in (1);
alter table parttable2 attach partition parttable3 for values in (1);

-
-INSERT a tuple into parttable3 which does not satisfy parttable1's 
partition constraint
-we will get an error
-
insert into parttable3 values(2,1,1);
ERROR:  new row for relation "parttable3" violates partition constraint
DETAIL:  Failing row contains (2, 1, 1).

-
- parttable1 is no longer the grandparent of parttable3.
- I thought the partition constraint of parttable1 does not matter anymore
-
alter table parttable1 detach partition parttable2;

-
-INSERT a tuple into parttable3 which does not satisfy parttable1's 
partition constraint
- *** I expect a successful insertion, but it returns an error again. ***
-
insert into parttable3 values(2,1,1);
ERROR:  new row for relation "parttable3" violates partition constraint
DETAIL:  Failing row contains (2, 1, 1).

RECONNECT
-
-Reconnect the postgres which will invalidate the relcache
- INSERT a tuple into parttable3 which does not satisfy parttable1's 
partition constraint
- We succeeded this time as expected.
-
insert into parttable3 values(2,1,1);
INSERT 0 1


Best regards,
houzj

Re: pgbench logging broken by time logic changes

2021-06-22 Thread Fabien COELHO



Hello,


+# note: this test is time sensitive, and may fail on a very
+#   loaded host.
+# note: --progress-timestamp is not tested
+my $delay = pgbench(
+   '-T 2 -P 1 -l --aggregate-interval=1 -S -b se@2'
+   . ' --rate=20 --latency-limit=1000 -j ' . $nthreads
+   . ' -c 3 -r',
+   0,
+   [   qr{type: multiple},
+   qr{clients: 3},
+   qr{threads: $nthreads},
+   qr{duration: 2 s},
+   qr{script 1: .* select only},
+   qr{script 2: .* select only},
+   qr{statement latencies in milliseconds},
+   qr{FROM pgbench_accounts} ],
+   [ qr{vacuum}, qr{progress: 1\b} ],
+   'pgbench progress', undef,
+   "--log-prefix=$bdir/001_pgbench_log_1");



Could we make that shorter at 1s?  That will shorten the duration of
the test run.  It is easy to miss that this test is for
--aggregate-interval (aka the logAgg() path) without a comment.


It is for -T, -P and --aggregate-interval. The units of all these is 
seconds, the minimum is 1, I put 2 so that It is pretty sure to get some 
output. We could try 1, but I'm less confident that an output is ensured 
in all cases on a very slow host which may decide to shorten the run 
before having shown a progress for instance.



+# cool check that we are around 2 seconds
+# The rate may results in an unlucky schedule which triggers
+# an early exit, hence the loose bound.
+ok(0.0 < $delay && $delay < 4.0, "-T 2 run around 2 seconds");

The command itself would not fail, but we would just fail on a machine
where the difference in start/stop time is higher than 4 seconds,
right?


Yep. It could detect a struck pgbench process which was one of the 
reported issue. Maybe there should be a timeout added.



On RPI-level machines, this could fail reliably.


Dunno, Not sure what RPI means. Probably not "Retail Price Index"… maybe 
Rasberry-Pi? In that case, the O-4 second leeway is intended to be loose 
enough to accomodate such hosts, but I cannot test that.


I am not completely sure what's the additional value we can get from 
that extra test, to be honest.


This would be to detect a somehow stuck process. It could be looser if 
necessary. Or removed, or preferably commented out, or enabled with some 
options (eg an environment variable? configure option?). Such control 
could also skip all 3 calls, in which case the 2 seconds is not an issue.



+# $nthreads threads, 2 seconds, but due to timing imprecision we might get
+# only 1 or as many as 3 progress reports per thread.
+check_pgbench_logs($bdir, '001_pgbench_log_1', $nthreads, 1, 3,
+   qr{^\d{10,} \d{1,2} \d+ \d+ \d+ \d+ \d+ \d+ \d+ \d+ \d+$});
+
Now this one is good and actually stable thanks to the fact that we'd
get at least the final logs, and the main complain we got to discuss
about on this thread was the format of the logs.


Yep, this test would have probably detected the epoch issue reported by 
Greg.



I would say to give up on the first test, and keep the second.


You mean remove the time check and keep the log check. I'd like to keep a 
time check, or at least have it in comment so that I can enable it simply.


But, is this regex correct?  Shouldn't we check for six integer fields 
only with the first one having a minimal number of digits for the epoch?


Epoch (seconds since 1970-01-01?) is currently 10 digits. Not sure how 
well it would work if some host have another zero start date.


Given the options of the bench run, there are that many fields in the 
log output, I'm not sure why we would want to check for less?


--
Fabien.

Re: [bug?] Missed parallel safety checks, and wrong parallel safety

2021-06-22 Thread Amit Kapila

On Wed, Jun 23, 2021 at 6:35 AM houzj.f...@fujitsu.com
 wrote:
>
> On Tuesday, June 22, 2021 8:25 PM Amit Kapila  wrote:
> > On Wed, Jun 16, 2021 at 8:57 AM houzj.f...@fujitsu.com 
> >  wrote:
> > >
> > > I think the check of partition could be even more complicated if we
> > > need to check the parallel safety of partition key expression. If user
> > > directly insert into a partition, then we need invoke
> > > ExecPartitionCheck which will execute all it's parent's and
> > > grandparent's partition key expressions. It means if we change a
> > > parent table's partition key expression(by 1) change function in expr
> > > or 2) attach the parent table as partition of another parent table), then 
> > > we
> > need to invalidate all its child's relcache.
> > >
> >
> > I think we already invalidate the child entries when we add/drop 
> > constraints on
> > a parent table. See ATAddCheckConstraint, ATExecDropConstraint. If I am not
> > missing anything then this case shouldn't be a problem. Do you have
> > something else in mind?
>
> Currently, attach/detach a partition doesn't invalidate the child entries
> recursively, except when detach a partition concurrently which will add a
> constraint to all the child. Do you mean we can add the logic about
> invalidating the child entries recursively when attach/detach a partition ?
>

I was talking about adding/dropping CHECK or other constraints on
partitioned tables via Alter Table. I think if attach/detach leads to
change in constraints of child tables then either they should
invalidate child rels to avoid problems in the existing sessions or if
it is not doing due to a reason then probably it might not matter. I
see that you have started a separate thread [1] to confirm the
behavior of attach/detach partition and we might want to decide based
on the conclusion of that thread.

[1] - 
https://www.postgresql.org/message-id/OS3PR01MB5718DA1C4609A25186D1FBF194089%40OS3PR01MB5718.jpnprd01.prod.outlook.com

-- 
With Regards,
Amit Kapila.

Re: [PoC] Federated Authn/z with OAUTHBEARER

2021-06-22 Thread Michael Paquier

On Tue, Jun 22, 2021 at 11:26:03PM +, Jacob Champion wrote:
> Done [1, 2]. I've copied your comments into those threads with my
> responses, and I'll have them registered in commitfest shortly.

Thanks!
--
Michael


signature.asc
Description: PGP signature

Re: Assertion failure in HEAD and 13 after calling COMMIT in a stored proc

2021-06-22 Thread Greg Nancarrow

On Tue, Jun 22, 2021 at 10:56 PM Ranier Vilela  wrote:
>
> On Mon, Jun 21, 2021 at 04:19:27PM -0700, Jim Nasby wrote:
> > The following generates an assertion failure. Quick testing with start and
> > stop as well as the core dump shows it’s failing on the execution of
> > `schema_name := schema_name(i)` immediately after COMMIT, because there’s no
> > active snapshot. On a build without asserts I get a failure in
> > GetActiveSnapshot() (second stack trace). This works fine on 12_STABLE, but
> > fails on 13_STABLE and HEAD.
>
> For me it's a typo.
> need_snapshot = (expr->expr_simple_mutable || !estate->readonly_func);
>
...
>
> The comments in the function are clear:
> If expression is mutable OR is a non-read-only function, so need a snapshot.
>

I have to agree with you.
Looks like the "&&" should really be an "||". The explanation in the
code comment is pretty clear on this, as you say.

I was able to reproduce the problem using your example. It produced a
coredump, pointing to the failed "Assert(ActiveSnapshotSet());" in
pg_plan_query().
I also verified that your patch seemed to fix the problem.

However, I found that this issue is masked by the following recent commit:

commit d102aafb6259a6a412803d4b1d8c4f00aa17f67e
Author: Tom Lane 
Date:   Tue Jun 22 17:48:39 2021 -0400
Restore the portal-level snapshot for simple expressions, too.

With this commit in place, there is an active snapshot in place
anyway, so for your example, that Assert no longer fires as it did
before.
However, I still think that your fix is valid and needed.

Regards,
Greg Nancarrow
Fujitsu Australia

Re: snapshot too old issues, first around wraparound and then more.

2021-06-22 Thread Greg Stark

On Thu, 17 Jun 2021 at 23:49, Noah Misch  wrote:
>
> On Wed, Jun 16, 2021 at 12:00:57PM -0400, Tom Lane wrote:
> > I agree that's a great use-case.  I don't like this implementation though.
> > I think if you want to set things up like that, you should draw a line
> > between the tables it's okay for the long transaction to touch and those
> > it isn't, and then any access to the latter should predictably draw an
> > error.

> I agree that would be a useful capability, but it solves a different problem.

Yeah, I think this discussion veered off into how to improve vacuum
snapshot tracking. That's an worthwhile endeavour but it doesn't
really address the use case this patch was there to target.

Fundamentally there's no way in SQL for users to give this information
to Postgres. There's nothing in SQL or our API that lets a client
inform Postgres what tables a session is going to access within a
transaction in the future.

What this alternative would look like would be a command that a client
would have to issue at the start of every transaction listing every
table that transaction will be allowed to touch. Any attempt to read
from any other table during the transaction would then get an error.

That sounds like it would be neat but it wouldn't work great with the
general approach in Postgres of having internal functions accessing
relations on demand (think of catalog tables, toast tables, and
pg_proc functions).

The "snapshot too old" approach is much more in line with Postgres's
general approach of giving users a general purpose platform and then
dealing gracefully with the consequences.

-- 
greg

Re: [HACKERS] logical decoding of two-phase transactions

2021-06-22 Thread vignesh C

On Wed, Jun 23, 2021 at 9:10 AM Ajin Cherian  wrote:
>
> On Tue, Jun 22, 2021 at 3:36 PM Greg Nancarrow  wrote:
>
> > Some minor comments:
> >
> > (1)
> > v88-0002
> >
> > doc/src/sgml/logicaldecoding.sgml
> >
> > "examples shows" is not correct.
> > I think there is only ONE example being referred to.
> >
> > BEFORE:
> > +The following examples shows how logical decoding is controlled over 
> > the
> > AFTER:
> > +The following example shows how logical decoding is controlled over the
> >
> >
> fixed.
>
> > (2)
> > v88 - 0003
> >
> > doc/src/sgml/ref/create_subscription.sgml
> >
> > (i)
> >
> > BEFORE:
> > +  to the subscriber on the PREPARE TRANSACTION. By default,
> > the transaction
> > +  prepared on publisher is decoded as a normal transaction at 
> > commit.
> > AFTER:
> > +  to the subscriber on the PREPARE TRANSACTION. By default,
> > the transaction
> > +  prepared on the publisher is decoded as a normal
> > transaction at commit time.
> >
>
> fixed.
>
> > (ii)
> >
> > src/backend/access/transam/twophase.c
> >
> > The double-bracketing is unnecessary:
> >
> > BEFORE:
> > + if ((gxact->valid && strcmp(gxact->gid, gid) == 0))
> > AFTER:
> > + if (gxact->valid && strcmp(gxact->gid, gid) == 0)
> >
>
> fixed.
>
> > (iii)
> >
> > src/backend/replication/logical/snapbuild.c
> >
> > Need to add some commas to make the following easier to read, and
> > change "needs" to "need":
> >
> > BEFORE:
> > + * The prepared transactions that were skipped because previously
> > + * two-phase was not enabled or are not covered by initial snapshot needs
> > + * to be sent later along with commit prepared and they must be before
> > + * this point.
> > AFTER:
> > + * The prepared transactions, that were skipped because previously
> > + * two-phase was not enabled or are not covered by initial snapshot, need
> > + * to be sent later along with commit prepared and they must be before
> > + * this point.
> >
>
> fixed.
>
> > (iv)
> >
> > src/backend/replication/logical/tablesync.c
> >
> > I think the convention used in Postgres code is to check for empty
> > Lists using "== NIL" and non-empty Lists using "!= NIL".
> >
> > BEFORE:
> > + if (table_states_not_ready && !last_start_times)
> > AFTER:
> > + if (table_states_not_ready != NIL && !last_start_times)
> >
> >
> > BEFORE:
> > + else if (!table_states_not_ready && last_start_times)
> > AFTER:
> > + else if (table_states_not_ready == NIL && last_start_times)
> >
>
> fixed.
>
> Also fixed comments from Vignesh:
>
> 1) This content is present in
> v87-0001-Add-option-to-set-two-phase-in-CREATE_REPLICATIO.patch and
> v87-0003-Add-support-for-prepared-transactions-to-built-i.patch, it
> can be removed from one of them
>
> +   TWO_PHASE
> +   
> +
> + Specify that this logical replication slot supports decoding
> of two-phase
> + transactions. With this option, two-phase commands like
> + PREPARE TRANSACTION, COMMIT
> PREPARED
> + and ROLLBACK PREPARED are decoded and 
> transmitted.
> + The transaction will be decoded and transmitted at
> + PREPARE TRANSACTION time.
> +
> +   
> +  
> +
> +  
>
> I don't see this duplicate content.

Thanks for the updated patch.
The patch v89-0001-Add-option-to-set-two-phase-in-CREATE_REPLICATIO.patch
has the following:
+   TWO_PHASE
+   
+
+ Specify that this logical replication slot supports decoding
of two-phase
+ transactions. With this option, two-phase commands like
+ PREPARE TRANSACTION, COMMIT
PREPARED
+ and ROLLBACK PREPARED are decoded and transmitted.
+ The transaction will be decoded and transmitted at
+ PREPARE TRANSACTION time.
+
+   
+  

The patch v89-0003-Add-support-for-prepared-transactions-to-built-i.patch
has the following:
+   TWO_PHASE
+   
+
+ Specify that this replication slot supports decode of two-phase
+ transactions. With this option, two-phase commands like
+ PREPARE TRANSACTION, COMMIT
PREPARED
+ and ROLLBACK PREPARED are decoded and transmitted.
+ The transaction will be decoded and transmitted at
+ PREPARE TRANSACTION time.
+
+   
+  

We can remove one of them.

Regards,
Vignesh

Automatic notification for top transaction IDs

2021-06-22 Thread Gurjeet Singh

I came across this thread [1] to disallow canceling a transaction not
yet confirmed by a synchronous replica. I think my proposed patch
might help that case as well, hence adding all involved in that thread
to BCC, for one-time notification.

As mentioned in that thread, when sending a cancellation signal, the
client cannot be sure if the cancel signal was honored, and if the
transaction was cancelled successfully. In the attached patch, the
backend emits a NotificationResponse containing the current full
transaction id. It does so only if the relevant GUC is enabled, and
when the top-transaction is being assigned the ID.

This information can be useful to the client, when:
i) it wants to cancel a transaction _after_ issuing a COMMIT, and
ii) it wants to check the status of its transaction that it sent
COMMIT for, but never received a response (perhaps because the server
crashed).

Additionally, this information can be useful for middleware, like
Transaction Processing Monitors, which can now transparently (without
any change in application code) monitor the status of transactions (by
watching for the transaction status indicator in the ReadyForQuery
protocol message). They can use the transaction ID from the
NotificationResponse to open a watcher, and on seeing either an 'E' or
'I' payload in subsequent ReadyForQuery messages, close the watcher.
On server crash, or other adverse events, they can then use the
transaction IDs still being watched to check status of those
transactions, and take appropriate actions, e.g. retry any aborted
transactions.

We cannot use the elog() mechanism for this notification because it is
sensitive to the value of client_min_messages. Hence I used the NOTIFY
infrastructure for this message. I understand that this usage violates
some expectations as to how NOTIFY messages are supposed to behave
(see [2] below), but I think these are acceptable violations; open to
hearing if/why this might not be acceptable, and any possible
alternatives.

I'm not very familiar with the parallel workers infrastructure, so the
patch is missing any consideration for those.

Reviews welcome.

[1]: subject was: Re: Disallow cancellation of waiting for synchronous
replication
thread:
https://www.postgresql.org/message-id/flat/C1F7905E-5DB2-497D-ABCC-E14D4DEE506C%40yandex-team.ru

[2]:
At present, NotificationResponse can only be sent outside a
transaction, and thus it will not occur in the middle of a
command-response series, though it might occur just before ReadyForQuery.
It is unwise to design frontend logic that assumes that, however.
Good practice is to be able to accept NotificationResponse at any
point in the protocol.

78 matches

Mail list logo