date:20210609

Re: BF assertion failure on mandrill in walsender, v13

2021-06-09 Thread Noah Misch

On Thu, Jun 10, 2021 at 10:47:20AM +1200, Thomas Munro wrote:
> Not sure if there is much chance of debugging this one-off failure in
> without a backtrace (long shot: any chance there's still a core
> file?)

No; it was probably in a directory deleted for each run.  One would need to
add dbx support to the buildfarm client, or perhaps add support for saving
build directories when there's a core, so I can operate dbx manually.

Re: Parallel INSERT SELECT take 2

2021-06-09 Thread Greg Nancarrow

On Thu, Jun 10, 2021 at 11:26 AM houzj.f...@fujitsu.com
 wrote:
>
> Through further review and thanks for greg-san's suggestions,
> I attached a new version patchset with some minor change in 0001,0003 and 
> 0004.
>
> 0001.
> * fix a typo in variable name.
> * add a TODO in patch comment about updating the version number when branch 
> PG15.
>
> 0003
> * fix a 'git apply white space' warning.
> * Remove some unnecessary if condition.
> * add some code comments above the safety check function.
> * Fix some typo.
>
> 0004
> * add a testcase to test ALTER PARALLEL DML UNSAFE/RESTRICTED.
>

Thanks,  those updates addressed most of what I was going to comment
on for the v9 patches.

Some additional comments on the v10 patches:

(1) I noticed some functions in the 0003 patch have no function header:

   make_safety_object
   parallel_hazard_walker
   target_rel_all_parallel_hazard_recurse

(2) I found the "recurse_partition" parameter of the
target_rel_all_parallel_hazard_recurse() function a bit confusing,
because the function recursively checks partitions without looking at
that flag. How about naming it "is_partition"?

(3) The names of the utility functions don't convey that they operate on tables.

How about:

   pg_get_parallel_safety() -> pg_get_table_parallel_safety()
   pg_get_max_parallel_hazard() -> pg_get_table_max_parallel_hazard()

or pg_get_rel_x()?

What do you think?

(4) I think that some of the tests need parallel dml settings to match
their expected output:

(i)
+-- Test INSERT with underlying query - and RETURNING (no projection)
+-- (should create a parallel plan; parallel SELECT)

-> but creates a serial plan (so needs to set parallel dml safe, so a
parallel plan is created)

(ii)
+-- Parallel INSERT with unsafe column default, should not use a parallel plan
+--
+alter table testdef parallel dml safe;

-> should set "unsafe" not "safe"

(iii)
+-- Parallel INSERT with restricted column default, should use parallel SELECT
+--
+explain (costs off) insert into testdef(a,b,d) select a,a*2,a*8 from test_data;

-> should use "alter table testdef parallel dml restricted;" before the explain

(iv)
+--
+-- Parallel INSERT with restricted and unsafe column defaults, should
not use a parallel plan
+--
+explain (costs off) insert into testdef(a,d) select a,a*8 from test_data;

-> should use "alter table testdef parallel dml unsafe;"  before the explain


Regards,
Greg Nancarrow
Fujitsu Australia

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

2021-06-09 Thread Amit Kapila

On Wed, Jun 9, 2021 at 9:47 PM Robert Haas  wrote:
>
> On Wed, Jun 9, 2021 at 2:43 AM Tom Lane  wrote:
> > There are specific cases where there's a good reason to worry.
> > For example, if we assume blindly that domain_in() is parallel
> > safe, we will have cause to regret that.  But I don't find that
> > to be a reason why we need to lock down everything everywhere.
> > We need to understand the tradeoffs involved in what we check,
> > and apply checks that are likely to avoid problems, while not
> > being too nanny-ish.
>
> Yeah, that's exactly how I feel about it, too.
>

Fair enough. So, I think there is a consensus to drop this patch and
if one wants then we can document these cases. Also, we don't want it
to enable parallelism for Inserts where we are trying to pursue the
approach to have a flag in pg_class which allows users to specify
whether writes are allowed on a specified relation.

-- 
With Regards,
Amit Kapila.

RE: locking [user] catalog tables vs 2pc vs logical rep

2021-06-09 Thread osumi.takami...@fujitsu.com

On Thursday, June 10, 2021 1:14 PM vignesh C 
> On Wed, Jun 9, 2021 at 12:03 PM osumi.takami...@fujitsu.com
>  wrote:
> >
> > On Wednesday, June 9, 2021 12:06 PM Amit Kapila
>  wrote:
> > > On Tue, Jun 8, 2021 at 6:24 PM vignesh C  wrote:
> > > >
> > > > Thanks for the updated patch.
> > > >
> > > > I have few comments:
> > > > 1) Should we list the actual system tables like
> > > > pg_class,pg_trigger, etc instead of any other catalog table?
> > > > User has issued an explicit LOCK on pg_class (or any other catalog
> > > > table)
> > > >
> > >
> > > I think the way it is mentioned is okay. We don't need to specify
> > > other catalog tables.
> > Okay.
> >
> >
> > > > 2) Here This means deadlock, after this we mention deadlock again
> > > > for each of the examples, we can remove it if redundant.
> > > > This can happen in the following ways:
> > I think this sentence works to notify that commands described below
> > are major scenarios naturally, to the readers. Then, I don't want to remove
> it.
> >
> > If you somehow feel that the descriptions are redundant, how about
> > unifying all listitems as nouns. like below ?
> >
> > * An explicit LOCK on
> > pg_class (or any other catalog table) in a
> > transaction
> > * Reordering pg_class by
> > CLUSTER command in a transaction
> > * Executing TRUNCATE on user_catalog_table
> >
> 
> This looks good to me. Keep the 2PC documentation patch also on the same
> lines.
Yeah, of course. Thanks for your confirmation.


Best Regards,
Takamichi Osumi

Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options

2021-06-09 Thread Michael Paquier

On Thu, Jun 10, 2021 at 09:17:55AM +0530, Bharath Rupireddy wrote:
> Hm. I get it. Unfortunately the commit b1ff33f is missing information
> on what the coverity tool was complaining of and it has no related
> discussion at all.

This came from a FORWARD_NULL complain, due to the fact that
parse_subscription_options() has checks for all three options if
connect is non-NULL a bit down after being done with the value
assignments with the DefElems.  So coverity was warning that we'd
better be careful to always have all three pointers set if a
connection is wanted by the caller.
--
Michael


signature.asc
Description: PGP signature

Re: locking [user] catalog tables vs 2pc vs logical rep

2021-06-09 Thread vignesh C

On Wed, Jun 9, 2021 at 12:03 PM osumi.takami...@fujitsu.com
 wrote:
>
> On Wednesday, June 9, 2021 12:06 PM Amit Kapila  
> wrote:
> > On Tue, Jun 8, 2021 at 6:24 PM vignesh C  wrote:
> > >
> > > Thanks for the updated patch.
> > >
> > > I have few comments:
> > > 1) Should we list the actual system tables like pg_class,pg_trigger,
> > > etc instead of any other catalog table?
> > > User has issued an explicit LOCK on pg_class (or any other catalog
> > > table)
> > >
> >
> > I think the way it is mentioned is okay. We don't need to specify other 
> > catalog
> > tables.
> Okay.
>
>
> > > 2) Here This means deadlock, after this we mention deadlock again for
> > > each of the examples, we can remove it if redundant.
> > > This can happen in the following ways:
> I think this sentence works to notify that commands described below
> are major scenarios naturally, to the readers. Then, I don't want to remove 
> it.
>
> If you somehow feel that the descriptions are redundant,
> how about unifying all listitems as nouns. like below ?
>
> * An explicit LOCK on pg_class 
> (or any other catalog table) in a transaction
> * Reordering pg_class by CLUSTER 
> command in a transaction
> * Executing TRUNCATE on user_catalog_table
>

This looks good to me. Keep the 2PC documentation patch also on the same lines.

Regards,
Vignesh

Re: logical replication of truncate command with trigger causes Assert

2021-06-09 Thread Amit Kapila

On Wed, Jun 9, 2021 at 8:44 PM Mark Dilger  wrote:
>
> > On Jun 9, 2021, at 7:52 AM, Tom Lane  wrote:
> >
> > Here's a draft patch for that.  I decided the most sensible way to
> > organize this is to pair the existing ensure_transaction() subroutine
> > with a cleanup subroutine.  Rather unimaginatively, perhaps, I renamed
> > it to begin_transaction_step and named the cleanup end_transaction_step.
> > (Better ideas welcome.)
>
> Thanks!  The regression test I posted earlier passes with this patch applied.
>

I have also read the patch and it looks good to me.

> > Somewhat unrelated, but ... am I reading the code correctly that
> > apply_handle_stream_start and related routines are using Asserts
> > to check that the remote sent stream-control messages in the correct
> > order?
> >

Yes. I think you are talking about Assert(!in_streamed_transaction).
There is no particular reason that such Asserts are required, so we
can change to test-and-elog as you suggested later in your email.

>  That seems many degrees short of acceptable.
>
> Even if you weren't reading that correctly, this bit:
>
> xid = pq_getmsgint(s, 4);
>
> Assert(TransactionIdIsValid(xid));
>
> simply asserts that the sending server didn't send an invalid subtransaction 
> id.
>

This also needs to be changed to test-and-elog.

-- 
With Regards,
Amit Kapila.

Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options

2021-06-09 Thread Bharath Rupireddy

On Thu, Jun 10, 2021 at 8:55 AM Peter Smith  wrote:
> > > 2.
> > > + /* If connect option is supported, the others also need to be. */
> > > + Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
> > > +(IsSet(supported_opts, SUBOPT_ENABLED) &&
> > > + IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
> > > + IsSet(supported_opts, SUBOPT_COPY_DATA)));
> > >
> > > This comment about "the others" doesn’t make sense to me.
> > >
> > > e.g. Why only these 3 options? What about all those other SUBOPT_* 
> > > options?
> >
> > It is an existing Assert and comment for ensuring somebody doesn't
> > call parse_subscription_options with SUBOPT_CONNECT, without
> > SUBOPT_ENABLED, SUBOPT_CREATE_SLOT and SUBOPT_COPY_DATA. In other
> > words, when SUBOPT_CONNECT is passed in, the other three options
> > should also be passed. " the others" there in the comment makes sense
> > just by looking at the Assert statement.
> >
>
> This misses the point of my question. And deducing the meaning of the
> "the others" from the code is completely backwards! The comment
> describes the code. The code doesn't describe the comment.
>
> Again, I was asking why “the others” are only these 3 options?. What
> about binary? What about streaming? What about refresh?
> In other words - what was the *intent* of that comment, and does the
> new code still meet the requirements of that intent? I think it does
> not.
>
> If you see github [1] when that code was first  implemented you can
> see that “the others” referred to every option other than the
> “connect”. At that time, the only others were those 3 - enabled,
> create_slot, copy_data. But now there are lots more options so
> something definitely needs to change.
> E.g.
> - Maybe the Assert now needs to include all the new options as well?
> - Maybe the entire reason for the Assert has become redundant now due
> to the introduction of SubOpts. It looks like it was not functional
> code - just something to quieten a static analysis tool.
> - Certainly “the others” is too vague and no longer has the original
> meaning anymore
>
> I don't know the answer; my guess is that all this has become obsolete
> and the whole Assert and the dodgy comment can just be deleted.

Hm. I get it. Unfortunately the commit b1ff33f is missing information
on what the coverity tool was complaining of and it has no related
discussion at all.

I agree to remove that assertion entirely. I will post a new patch set soon.

> > > 3.
> > > I feel that this patch should be split into 2 parts
> > > a) the SubOpts changes, and
> > > b) the mutually exclusive options change.
> >
> > Divided the patch into two.
> >
> > > I agree that the new SubOpts struct etc. is an improvement over existing 
> > > code.
> > >
> > > But, for the mutually exclusive options part I don't see what is
> > > gained by the new patch code. I preferred the old code with its
> > > multiple ereports. Although it was a bit repetitive IMO it was easier
> > > to read that way, and length-wise there is almost no difference. So if
> > > it is less readable and not a lot shorter then what is the benefit of
> > > the change?
> >
> > I personally don't like the repeated code when there's a chance of
> > doing it better. It might not reduce the loc, but it removes the many
> > similar ereport(ERROR calls. PSA v4-0002 patch. I think the committer
> > can take a call on it.
> >
>
> Thanks for splitting them. My votes are +1 for patch 0001 and  -1 for
> patch 0002. As you say, someone else can decide.

Let's see how it goes further.

With Regards,
Bharath Rupireddy.

Re: Refactor "mutually exclusive options" error reporting code in parse_subscription_options

2021-06-09 Thread Peter Smith

On Thu, Jun 10, 2021 at 1:28 AM Bharath Rupireddy
 wrote:
>
> On Wed, Jun 9, 2021 at 10:37 AM Peter Smith  wrote:
> >
[...]

I checked the v4* patches.
Everything applies and builds and tests OK for me.

> > 2.
> > + /* If connect option is supported, the others also need to be. */
> > + Assert(!IsSet(supported_opts, SUBOPT_CONNECT) ||
> > +(IsSet(supported_opts, SUBOPT_ENABLED) &&
> > + IsSet(supported_opts, SUBOPT_CREATE_SLOT) &&
> > + IsSet(supported_opts, SUBOPT_COPY_DATA)));
> >
> > This comment about "the others" doesn’t make sense to me.
> >
> > e.g. Why only these 3 options? What about all those other SUBOPT_* options?
>
> It is an existing Assert and comment for ensuring somebody doesn't
> call parse_subscription_options with SUBOPT_CONNECT, without
> SUBOPT_ENABLED, SUBOPT_CREATE_SLOT and SUBOPT_COPY_DATA. In other
> words, when SUBOPT_CONNECT is passed in, the other three options
> should also be passed. " the others" there in the comment makes sense
> just by looking at the Assert statement.
>

This misses the point of my question. And deducing the meaning of the
"the others" from the code is completely backwards! The comment
describes the code. The code doesn't describe the comment.

Again, I was asking why “the others” are only these 3 options?. What
about binary? What about streaming? What about refresh?
In other words - what was the *intent* of that comment, and does the
new code still meet the requirements of that intent? I think it does
not.

If you see github [1] when that code was first  implemented you can
see that “the others” referred to every option other than the
“connect”. At that time, the only others were those 3 - enabled,
create_slot, copy_data. But now there are lots more options so
something definitely needs to change.
E.g.
- Maybe the Assert now needs to include all the new options as well?
- Maybe the entire reason for the Assert has become redundant now due
to the introduction of SubOpts. It looks like it was not functional
code - just something to quieten a static analysis tool.
- Certainly “the others” is too vague and no longer has the original
meaning anymore

I don't know the answer; my guess is that all this has become obsolete
and the whole Assert and the dodgy comment can just be deleted.

> > 3.
> > I feel that this patch should be split into 2 parts
> > a) the SubOpts changes, and
> > b) the mutually exclusive options change.
>
> Divided the patch into two.
>
> > I agree that the new SubOpts struct etc. is an improvement over existing 
> > code.
> >
> > But, for the mutually exclusive options part I don't see what is
> > gained by the new patch code. I preferred the old code with its
> > multiple ereports. Although it was a bit repetitive IMO it was easier
> > to read that way, and length-wise there is almost no difference. So if
> > it is less readable and not a lot shorter then what is the benefit of
> > the change?
>
> I personally don't like the repeated code when there's a chance of
> doing it better. It might not reduce the loc, but it removes the many
> similar ereport(ERROR calls. PSA v4-0002 patch. I think the committer
> can take a call on it.
>

Thanks for splitting them. My votes are +1 for patch 0001 and  -1 for
patch 0002. As you say, someone else can decide.

--
[1] 
https://github.com/postgres/postgres/commit/b1ff33fd9bb82937f4719f264972e6a3c83cdb89#

Kind Regards,
Peter Smith
Fujitsu Australia.

Re: Multiple hosts in connection string failed to failover in non-hot standby mode

2021-06-09 Thread Tom Lane

Michael Paquier  writes:
> On Wed, Jun 09, 2021 at 12:05:10PM -0400, Tom Lane wrote:
>> Here's a draft patch that renames regress_ecpg_user2 to ecpg2_regression,

> Using ecpg2_regression for the role goes a bit against the recent rule
> to not create any role not suffixed by "regress_" as part of the
> regression tests, but I am fine to live with that here.

Oh dear, I forgot to check that carefully.  I'd been thinking the rule was
that such names must *contain* "regress", but looking at user.c, it's
stricter:

#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
if (strncmp(stmt->role, "regress_", 8) != 0)
elog(WARNING, "roles created by regression test cases should 
have names starting with \"regress_\"");
#endif

Meanwhile, the rule for database names is:

#ifdef ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS
if (IsUnderPostmaster && strstr(dbname, "regression") == NULL)
elog(WARNING, "databases created by regression test cases 
should have names including \"regression\"");
#endif

So unless we want to relax one or both of those, we can't have a user
name that matches the database name.

Now I'm inclined to go back to the first-draft patch I had, which just
dropped the first problematic test case, and added gssencmode=disable
to the second one.

regards, tom lane

Re: Patch: Range Merge Join

2021-06-09 Thread David Rowley

On Thu, 10 Jun 2021 at 03:05, Thomas wrote:
> We have implemented the Range Merge Join algorithm by extending the
> existing Merge Join to also support range conditions, i.e., BETWEEN-AND
> or @> (containment for range types).

It shouldn't be a blocker for you, but just so you're aware, there was
a previous proposal for this in [1] and a patch in [2]. I've include
Jeff here just so he's aware of this. Jeff may wish to state his
intentions with his own patch. It's been a few years now.

I only just glanced over the patch. I'd suggest getting rid of the /*
Thomas */ comments. We use git, so if you need an audit trail about
changes then you'll find it in git blame. If you have those for an
internal audit trail then you should consider using git. No committer
would commit those to PostgreSQL, so they might as well disappear.

For further review, please add the patch to the July commitfest [3].
We should be branching for pg15 sometime before the start of July.
There will be more focus on new patches around that time. Further
details in [4].

Also, I see this if your first post to this list, so welcome, and
thank you for the contribution. Also, just to set expectations;
patches like this almost always take a while to get into shape for
PostgreSQL. Please expect a lot of requests to change things. That's
fairly standard procedure. The process often drags on for months and
in some less common cases, years.

David

[1]
https://www.postgresql.org/message-id/flat/6227.1334559170%40sss.pgh.pa.us#82c771950ba486dec911923a5e91
[2]
https://www.postgresql.org/message-id/flat/CAMp0ubfwAFFW3O_NgKqpRPmm56M4weTEXjprb2gP_NrDaEC4Eg%40mail.gmail.com
[3] https://commitfest.postgresql.org/33/
[4] https://wiki.postgresql.org/wiki/CommitFest

1 2 >

1 - 100 of 111 matches

Mail list logo