Re: Planning counters in pg_stat_statements (using pgss_store)

[Julien Rouhaud]

On Fri, May 22, 2020 at 9:27 PM legrand legrand
 wrote:
>
> >> If we can store the plan for each statement, e.g., like pg_store_plans
> >> extension [1] does, rather than such partial information, which would
> >> be enough for your cases?
>
> > That'd definitely address way more use cases.  Do you know if some
> > benchmark were done to see how much overhead such an extension adds?
>
> Hi Julien,
> Did you asked about how overhead Auto Explain adds ?

Well, yes but on the other hand auto_explain is by design definitely
not something intended to trace all queries in an OLTP environment,
but rather configured to catch only some long running queries, so in
such cases the overhead is quite negligible.

> The only extension that was proposing to store plans with a decent planid
> calculation was pg_stat_plans that is not compatible any more with recent
> pg versions for years.

Ah I see.  AFAICT it's mainly missing the new node changes, but the
approach should otherwise still work smoothly.

Did you do some benchmark to compare this extension with the other
alternatives? Assuming that there's postgres version compatible with
all the extensions of course.

> We all know here that pg_store_plans, pg_show_plans, (my) pg_stat_sql_plans
> use ExplainPrintPlan through Executor Hook, and that Explain is slow ...
>
> Explain is slow because it was not designed for performances:
> 1/ colname_is_unique
> see
> https://www.postgresql-archive.org/Re-Explain-is-slow-with-tables-having-many-columns-td6047284.html
>
> 2/ hash_create from set_rtable_names
> Look with perf top about
>do $$ declare i int; begin for i in 1..100 loop execute 'explain
> select 1'; end loop end; $$;
>
> I may propose a "minimal" explain that only display explain's backbone and
> is much faster
> see
> https://github.com/legrandlegrand/pg_stat_sql_plans/blob/perf-explain/pgssp_explain.c
>
> 3/ All those extensions rebuild the explain output even with cached plan
> queries ...
>  a way to optimize this would be to build a planid during planning (using
> associated hook)
>
> 4/ All thoses extensions try to rebuild the explain plan even for trivial
> queries/plans
> like "select 1" or " insert into t values (,,,)" and that's not great for
> high transactional
> applications ...
>
> So yes, pg_store_plans is one of the short term answers to Andy Fan needs,
> the answer for the long term would be to help extensions to build planid and
> store plans,
> by **adding a planid field in plannedstmt memory structure ** and/or
> optimizing explain command;o)

I'd be in favor of adding a planid and using the same approach as
pg_store_plans.

Re: race condition when writing pg_control

[Michael Paquier]

On Sat, May 23, 2020 at 01:00:17AM +0900, Fujii Masao wrote:
> Per my quick check, XLogReportParameters() seems to have the similar issue,
> i.e., it updates the control file without taking ControlFileLock.
> Maybe we should fix this at the same time?

Yeah.  It also checks the control file values, implying that we should
have LW_SHARED taken at least at the beginning, but this lock cannot
be upgraded we need LW_EXCLUSIVE the whole time.  I am wondering if we
should check with an assert if ControlFileLock is taken when going
through UpdateControlFile().  We have one code path at the beginning
of redo where we don't need a lock close to the backup_label file
checks, but we could just pass down a boolean flag to the routine to
handle that case.  Another good thing in having an assert is that any
new caller of UpdateControlFile() would need to think about the need
of a lock.
--
Michael


signature.asc
Description: PGP signature

Re: Adding missing object access hook invocations

[Michael Paquier]

On Thu, May 21, 2020 at 09:32:55AM +0900, Michael Paquier wrote:
> Thanks for the input, Robert.  So, even if we are post-beta1 it looks
> like there are more upsides than downsides to get that stuff done
> sooner than later.  I propose to get that applied in the next couple
> of days, please let me know if there are any objections.

Hearing nothing, done.  Thanks all for the discussion.
--
Michael


signature.asc
Description: PGP signature

Re: Parallel Seq Scan vs kernel read ahead

[Amit Kapila]

On Sat, May 23, 2020 at 12:00 AM Robert Haas  wrote:
>
> On Tue, May 19, 2020 at 10:23 PM Amit Kapila  wrote:
> > Good experiment.  IIRC, we have discussed a similar idea during the
> > development of this feature but we haven't seen any better results by
> > allocating in ranges on the systems we have tried.  So, we want with
> > the current approach which is more granular and seems to allow better
> > parallelism.  I feel we need to ensure that we don't regress
> > parallelism in existing cases, otherwise, the idea sounds promising to
> > me.
>
> I think there's a significant difference. The idea I remember being
> discussed at the time was to divide the relation into equal parts at
> the very start and give one part to each worker.
>

I have checked the archives and found that we have done some testing
by allowing each worker to work on a block-by-block basis and by
having a fixed number of chunks for each worker.  See the results [1]
(the program used is attached in another email [2]).  The conclusion
was that we didn't find much difference with any of those approaches.
Now, the reason could be that because we have tested on a machine (I
think it was hydra (Power-7)) where the chunk-size doesn't matter but
I think it can show some difference in the machines on which Thomas
and David are testing.  At that time there was also a discussion to
chunk on the basis of "each worker processes one 1GB-sized segment"
which Tom and Stephen seem to support [3].  I think an idea to divide
the relation into segments based on workers for a parallel scan has
been used by other database (DynamoDB) as well [4] so it is not
completely without merit.  I understand that larger sized chunks can
lead to unequal work distribution but they have their own advantages,
so we might want to get the best of both the worlds where in the
beginning we have larger sized chunks and then slowly reduce the
chunk-size towards the end of the scan.  I am not sure what is the
best thing to do here but maybe some experiments can shed light on
this mystery.


[1] - 
https://www.postgresql.org/message-id/CAA4eK1JHCmN2X1LjQ4bOmLApt%2BbtOuid5Vqqk5G6dDFV69iyHg%40mail.gmail.com
[2] - 
https://www.postgresql.org/message-id/CAA4eK1JyVNEBE8KuxKd3bJhkG6tSbpBYX_%2BZtP34ZSTCSucA1A%40mail.gmail.com
[3] - https://www.postgresql.org/message-id/30549.1422459647%40sss.pgh.pa.us
[4] - 
https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/Scan.html#Scan.ParallelScan

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: xid wraparound danger due to INDEX_CLEANUP false

[Peter Geoghegan]

On Fri, May 22, 2020 at 1:40 PM Peter Geoghegan  wrote:
> It. seems like the right direction to me. Robert?

BTW, this is an interesting report of somebody using the INDEX_CLEANUP
feature when they had to deal with a difficult production issue:

https://www.buildkitestatus.com/incidents/h0vnx4gp7djx

This report is not really relevant to our discussion, but I thought
you might find it interesting.

-- 
Peter Geoghegan

Re: segmentation fault using currtid and partitioned tables

[Alvaro Herrera]

On 2020-Apr-09, Michael Paquier wrote:

> Playing more with this stuff, it happens that we have zero code
> coverage for currtid() and currtid2(), and the main user of those
> functions I can find around is the ODBC driver:
> https://coverage.postgresql.org/src/backend/utils/adt/tid.c.gcov.html

Yeah, they're there solely for ODBC as far as I know.

> There are multiple cases to consider, particularly for views:
> - Case of a view with ctid as attribute taken from table.
> - Case of a view with ctid as attribute with incorrect attribute
> type.
> It is worth noting that all those code paths can trigger various
> elog() errors, which is not something that a user should be able to do
> using a SQL-callable function.  There are also two code paths for
> cases where a view has no or more-than-one SELECT rules, which cannot
> normally be reached.

> All in that, I propose something like the attached to patch the
> surroundings with tests to cover everything I could think of, which I
> guess had better be backpatched?

I don't know, but this stuff is so unused that your patch seems
excessive ... and I think we'd rather not backpatch something so large.
I propose we do something less invasive in the backbranches, like just
throw elog() errors (nothing fancy) where necessary to avoid the
crashes.  Even for pg12 it seems that that should be sufficient.

For pg13 and beyond, I liked Tom's idea of installing dummy functions
for tables without storage -- that seems safer.

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

Re: password_encryption default

[Jonathan S. Katz]

On 5/22/20 5:21 PM, Tom Lane wrote:
> Vik Fearing  writes:
>> On 5/22/20 9:09 PM, Jonathan S. Katz wrote:
>>> As someone who is an unabashed SCRAM fan and was hoping the default
>>> would be up'd for v13, I would actually +1 making it the default in v14,
>>> i.e. because 9.5 will be EOL at that point, and as such we both have
>>> every* driver supporting SCRAM AND every version of PostgreSQL
>>> supporting SCRAM.
> 
>> Wasn't SCRAM introduced in 10?
> 
> Yeah.  But there's still something to Jonathan's argument, because 9.6
> will go EOL in November 2021, which is pretty close to when v14 will
> reach public release (assuming we can hold to the typical schedule).
> If we do it in v13, there'll be a full year where still-supported
> versions of PG can't do SCRAM, implying that clients would likely
> fail to connect to an up-to-date server.

^ that's what I meant.

Jonathan



signature.asc
Description: OpenPGP digital signature

Re: password_encryption default

[Tom Lane]

Vik Fearing  writes:
> On 5/22/20 9:09 PM, Jonathan S. Katz wrote:
>> As someone who is an unabashed SCRAM fan and was hoping the default
>> would be up'd for v13, I would actually +1 making it the default in v14,
>> i.e. because 9.5 will be EOL at that point, and as such we both have
>> every* driver supporting SCRAM AND every version of PostgreSQL
>> supporting SCRAM.

> Wasn't SCRAM introduced in 10?

Yeah.  But there's still something to Jonathan's argument, because 9.6
will go EOL in November 2021, which is pretty close to when v14 will
reach public release (assuming we can hold to the typical schedule).
If we do it in v13, there'll be a full year where still-supported
versions of PG can't do SCRAM, implying that clients would likely
fail to connect to an up-to-date server.

regards, tom lane

Re: xid wraparound danger due to INDEX_CLEANUP false

[Peter Geoghegan]

On Mon, May 18, 2020 at 7:32 PM Masahiko Sawada
 wrote:
> I've attached WIP patch for HEAD. With this patch, the core pass
> index_cleanup to bulkdelete and vacuumcleanup callbacks so that they
> can make decision whether run vacuum or not.
>
> If the direction of this patch seems good, I'll change the patch so
> that we pass something information to these callbacks indicating
> whether this vacuum is anti-wraparound vacuum. This is necessary
> because it's enough to invoke index cleanup before XID wraparound as
> per discussion so far.

It. seems like the right direction to me. Robert?

-- 
Peter Geoghegan

Re: password_encryption default

[Vik Fearing]

On 5/22/20 9:09 PM, Jonathan S. Katz wrote:
> As someone who is an unabashed SCRAM fan and was hoping the default
> would be up'd for v13, I would actually +1 making it the default in v14,
> i.e. because 9.5 will be EOL at that point, and as such we both have
> every* driver supporting SCRAM AND every version of PostgreSQL
> supporting SCRAM.

Wasn't SCRAM introduced in 10?
-- 
Vik Fearing

Re: Planning counters in pg_stat_statements (using pgss_store)

[legrand legrand]

>> If we can store the plan for each statement, e.g., like pg_store_plans
>> extension [1] does, rather than such partial information, which would
>> be enough for your cases?

> That'd definitely address way more use cases.  Do you know if some
> benchmark were done to see how much overhead such an extension adds?

Hi Julien,
Did you asked about how overhead Auto Explain adds ?

The only extension that was proposing to store plans with a decent planid 
calculation was pg_stat_plans that is not compatible any more with recent 
pg versions for years.

We all know here that pg_store_plans, pg_show_plans, (my) pg_stat_sql_plans
use ExplainPrintPlan through Executor Hook, and that Explain is slow ...

Explain is slow because it was not designed for performances:
1/ colname_is_unique
see
https://www.postgresql-archive.org/Re-Explain-is-slow-with-tables-having-many-columns-td6047284.html

2/ hash_create from set_rtable_names
Look with perf top about
   do $$ declare i int; begin for i in 1..100 loop execute 'explain
select 1'; end loop end; $$;

I may propose a "minimal" explain that only display explain's backbone and
is much faster
see
https://github.com/legrandlegrand/pg_stat_sql_plans/blob/perf-explain/pgssp_explain.c
 
3/ All those extensions rebuild the explain output even with cached plan
queries ...
 a way to optimize this would be to build a planid during planning (using
associated hook)

4/ All thoses extensions try to rebuild the explain plan even for trivial
queries/plans 
like "select 1" or " insert into t values (,,,)" and that's not great for
high transactional 
applications ...

So yes, pg_store_plans is one of the short term answers to Andy Fan needs, 
the answer for the long term would be to help extensions to build planid and
store plans, 
by **adding a planid field in plannedstmt memory structure ** and/or 
optimizing explain command;o)

Regards
PAscal



--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: password_encryption default

[Jonathan S. Katz]

On 5/22/20 11:34 AM, Tom Lane wrote:
> Stephen Frost  writes:
>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>>> As far as that last goes, we *did* get the buildfarm fixed to be all
>>> v11 scripts, so I thought we were ready to move forward on trying
>>> 09f08930f again.  It's too late to consider that for v13, but
>>> perhaps it'd be reasonable to change the SCRAM default now?  Not sure.
> 
>> I feel like it is.  I'm not even sure that I agree that it's really too
>> late to consider 09f08930f considering that's it's a pretty minor code
>> change and the up-side to that is having reasonable defaults out of the
>> box, as it were, something we have *long* been derided for.
> 
> Well, the argument against changing right now is that it would invalidate
> portability testing done against beta1, which users would be justifiably
> upset about.
> 
> I'm +1 for changing both of these things as soon as we branch for v14,
> but I feel like it's a bit late for v13.  If we aren't feature-frozen
> now, when will we be?

As someone who is an unabashed SCRAM fan and was hoping the default
would be up'd for v13, I would actually +1 making it the default in v14,
i.e. because 9.5 will be EOL at that point, and as such we both have
every* driver supporting SCRAM AND every version of PostgreSQL
supporting SCRAM.

(Would I personally love to do it sooner? Yes...but I think the stars
align for doing it in v14).

Jonathan



signature.asc
Description: OpenPGP digital signature

Re: Parallel Seq Scan vs kernel read ahead

[Robert Haas]

On Thu, May 21, 2020 at 6:28 PM Thomas Munro  wrote:
> Right, I think it's safe.  I think you were probably right that
> ramp-up isn't actually useful though, it's only the end of the scan
> that requires special treatment so we don't get unfair allocation as
> the work runs out, due to course grain.

The ramp-up seems like it might be useful if the query involves a LIMIT.

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

Re: Parallel Seq Scan vs kernel read ahead

[Robert Haas]

On Tue, May 19, 2020 at 10:23 PM Amit Kapila  wrote:
> Good experiment.  IIRC, we have discussed a similar idea during the
> development of this feature but we haven't seen any better results by
> allocating in ranges on the systems we have tried.  So, we want with
> the current approach which is more granular and seems to allow better
> parallelism.  I feel we need to ensure that we don't regress
> parallelism in existing cases, otherwise, the idea sounds promising to
> me.

I think there's a significant difference. The idea I remember being
discussed at the time was to divide the relation into equal parts at
the very start and give one part to each worker. I think that carries
a lot of risk of some workers finishing much sooner than others. This
idea, AIUI, is to divide the relation into chunks that are small
compared to the size of the relation, but larger than 1 block. That
carries some risk of an unequal division of work, as has already been
noted, but it's much less, especially if we use smaller chunk sizes
once we get close to the end, as proposed here.

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

Re: some grammar refactoring

[Mark Dilger]

> On May 18, 2020, at 11:43 PM, Peter Eisentraut 
>  wrote:
> 
> Here is a series of patches to do some refactoring in the grammar around the 
> commands COMMENT, DROP, SECURITY LABEL, and ALTER EXTENSION ... ADD/DROP.  In 
> the grammar, these commands (with some exceptions) basically just take a 
> reference to an object and later look it up in C code.  Some of that was 
> already generalized individually for each command (drop_type_any_name, 
> drop_type_name, etc.).  This patch combines it into common lists for all 
> these commands.
> 
> Advantages:
> 
> - Avoids having to list each object type at least four times.
> 
> - Object types not supported by security labels or extensions are now 
> explicitly listed and give a proper error message.  Previously, this was just 
> encoded in the grammar itself and specifying a non-supported object type 
> would just give a parse error.
> 
> - Reduces lines of code in gram.y.
> 
> - Removes some old cruft.

I like the general direction you are going with this, but the decision in 
v1-0006 to move the error for invalid object types out of gram.y and into 
extension.c raises an organizational question.   At some places in gram.y, 
there is C code that checks parsed tokens and ereports if they are invalid, in 
some sense extending the grammar right within gram.y.  In many other places, 
including what you are doing in this patch, the token is merely stored in a 
Stmt object with the error checking delayed until command processing.  For 
tokens which need to be checked against the catalogs, that decision makes 
perfect sense.  But for ones where all the information necessary to validate 
the token exists in the parser, it is not clear to me why it gets delayed until 
command processing.  Is there a design principle behind when these checks are 
done in gram.y vs. when they are delayed to the command processing?  I'm 
guessing in v1-0006 that you are doing it this way because there are multiple 
places in gram.y where tokens would need to be checked, and by delaying the 
check until ExecAlterExtensionContentsStmt, you can put the check all in one 
place.  Is that all it is?

I have had reason in the past to want to reorganize gram.y to have all these 
types of checks in a single, consistent format and location, rather than 
scattered through gram.y and backend/commands/.  Does anybody else have an 
interest in this?

My interest in this stems from the fact that bison can be run to generate data 
files that can then be used in reverse to generate random SQL.  The more the 
parsing logic is visible to bison, the more useful the generated data files 
are.  But a single, consistent design for extra-grammatical error checks could 
help augment those files fairly well, too.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Planning counters in pg_stat_statements (using pgss_store)

[Julien Rouhaud]

On Fri, May 22, 2020 at 3:51 PM Fujii Masao  wrote:
>
> On 2020/05/22 15:10, Andy Fan wrote:
> >
> >
> > On Thu, May 21, 2020 at 3:49 PM Julien Rouhaud  > > wrote:
> >
> > Le jeu. 21 mai 2020 à 09:17, Michael Paquier  > > a écrit :
> >
> > On Thu, May 21, 2020 at 08:49:53AM +0200, Julien Rouhaud wrote:
> >  > On Tue, May 19, 2020 at 4:29 AM Andy Fan 
> > mailto:zhihui.fan1...@gmail.com>> wrote:
> >  >> Thanks for the excellent extension. I want to add 5 more fields 
> > to satisfy the
> >  >> following requirements.
> >  >>
> >  >> int   subplan; /* No. of subplan in this query */
> >  >> int   subquery; /* No. of subquery */
> >  >> int   joincnt; /* How many relations are joined */
> >  >> bool hasagg; /* if we have agg function in this query */
> >  >> bool hasgroup; /* has group clause */
> >  >
> >  > Most of those fields can be computed using the raw sql 
> > satements.  Why
> >  > not adding functions like query_has_agg(querytext) to get the
> >  > information from pgss stored query text instead?
> >
> > Yeah I personally find concepts related only to the query string
> > itself not something that needs to be tied to pg_stat_statements.
> > ...
> >
> >
> > Indeed cte will bring additional concerns about the fields semantics. 
> > That's another good reason to go with external functions so you can add 
> > extra parameters for that if needed.
> >
> >
> > There are something more we can't get from query string easily. like:
> > 1. view involved.   2. subquery are pulled up so there is not subquery
> > indeed.  3. sublink are pull-up or become as an InitPlan rather than 
> > subPlan.
> > 4. joins are removed by remove_useless_joins.
>
> If we can store the plan for each statement, e.g., like pg_store_plans
> extension [1] does, rather than such partial information, which would
> be enough for your cases?

That'd definitely address way more use cases.  Do you know if some
benchmark were done to see how much overhead such an extension adds?

Re: pg_bsd_indent and -Wimplicit-fallthrough

[Julien Rouhaud]

On Thu, May 21, 2020 at 7:39 PM Piotr Stefaniak
 wrote:
>
> On 18/05/2020 11.22, Julien Rouhaud wrote:
> > On Sun, May 17, 2020 at 2:32 AM Michael Paquier  wrote:
> >>
> >> On Sat, May 16, 2020 at 11:56:28AM -0400, Tom Lane wrote:
> >>> In the meantime, I went ahead and pushed this to our pg_bsd_indent repo.
> >>
> >> Thanks, Tom.
> >
> > +1, thanks a lot!
> >
>
> Committed upstream, thank you.

Thanks!

Re: race condition when writing pg_control

[Fujii Masao]

On 2020/05/22 13:51, Thomas Munro wrote:

On Tue, May 5, 2020 at 9:51 AM Thomas Munro  wrote:

On Tue, May 5, 2020 at 5:53 AM Bossart, Nathan  wrote:

I believe I've discovered a race condition between the startup and
checkpointer processes that can cause a CRC mismatch in the pg_control
file.  If a cluster crashes at the right time, the following error
appears when you attempt to restart it:

 FATAL:  incorrect checksum in control file

This appears to be caused by some code paths in xlog_redo() that
update ControlFile without taking the ControlFileLock.  The attached
patch seems to be sufficient to prevent the CRC mismatch in the
control file, but perhaps this is a symptom of a bigger problem with
concurrent modifications of ControlFile->checkPointCopy.nextFullXid.


This does indeed look pretty dodgy.  CreateRestartPoint() running in
the checkpointer does UpdateControlFile() to compute a checksum and
write it out, but xlog_redo() processing
XLOG_CHECKPOINT_{ONLINE,SHUTDOWN} modifies that data without
interlocking.  It looks like the ancestors of that line were there
since 35af5422f64 (2006), but back then RecoveryRestartPoint() ran
UpdateControLFile() directly in the startup process (immediately after
that update), so no interlocking problem.  Then in cdd46c76548 (2009),
RecoveryRestartPoint() was split up so that CreateRestartPoint() ran
in another process.


Here's a version with a commit message added.  I'll push this to all
releases in a day or two if there are no objections.


+1 to push the patch.

Per my quick check, XLogReportParameters() seems to have the similar issue,
i.e., it updates the control file without taking ControlFileLock.
Maybe we should fix this at the same time?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: password_encryption default

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> I'm +1 for changing both of these things as soon as we branch for v14,
> >> but I feel like it's a bit late for v13.  If we aren't feature-frozen
> >> now, when will we be?
> 
> > I really don't consider changing of defaults to be on the same level as
> > implementation of whole features, even if changing those defaults
> > requires a few lines of code to go with the change.
> 
> The buildfarm fiasco with 09f08930f should remind us that changing
> defaults *does* break things, even if theoretically it shouldn't.
> At this phase of the v13 cycle, we should be looking to fix bugs,
> not to break more stuff.

Sure it does- for the special case of the buildfarm, and that takes
buildfarm code to fix.  Having users make changes to whatever scripts
they're using with PG between major versions is certainly not
unreasonable, or even between beta and final.  These things are not set
in stone at this point, they're the defaults, and it's beta time now,
not post release or RC.

If it breaks for regular users who are using the system properly then we
want to know about that and we'd ideally like to get that fixed before
the release.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: password_encryption default

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> I'm +1 for changing both of these things as soon as we branch for v14,
>> but I feel like it's a bit late for v13.  If we aren't feature-frozen
>> now, when will we be?

> I really don't consider changing of defaults to be on the same level as
> implementation of whole features, even if changing those defaults
> requires a few lines of code to go with the change.

The buildfarm fiasco with 09f08930f should remind us that changing
defaults *does* break things, even if theoretically it shouldn't.
At this phase of the v13 cycle, we should be looking to fix bugs,
not to break more stuff.

regards, tom lane

Re: password_encryption default

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> As far as that last goes, we *did* get the buildfarm fixed to be all
> >> v11 scripts, so I thought we were ready to move forward on trying
> >> 09f08930f again.  It's too late to consider that for v13, but
> >> perhaps it'd be reasonable to change the SCRAM default now?  Not sure.
> 
> > I feel like it is.  I'm not even sure that I agree that it's really too
> > late to consider 09f08930f considering that's it's a pretty minor code
> > change and the up-side to that is having reasonable defaults out of the
> > box, as it were, something we have *long* been derided for.
> 
> Well, the argument against changing right now is that it would invalidate
> portability testing done against beta1, which users would be justifiably
> upset about.

I don't think we're in complete agreement about the amount of
portability testing that's done with our beta source builds.  To that
point, however, the lack of such testing happening, if there is a lack,
is on us just as much as anyone else- we should be testing, to the
extent possible, as many variations of our configuration options as we
can across as many platforms as we can in the buildfarm.  If a
non-default setting doesn't work on one platform or another, that's a
bug to fix regardless and doesn't really impact the question of "what
should be the default".

> I'm +1 for changing both of these things as soon as we branch for v14,
> but I feel like it's a bit late for v13.  If we aren't feature-frozen
> now, when will we be?

I really don't consider changing of defaults to be on the same level as
implementation of whole features, even if changing those defaults
requires a few lines of code to go with the change.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: password_encryption default

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> As far as that last goes, we *did* get the buildfarm fixed to be all
>> v11 scripts, so I thought we were ready to move forward on trying
>> 09f08930f again.  It's too late to consider that for v13, but
>> perhaps it'd be reasonable to change the SCRAM default now?  Not sure.

> I feel like it is.  I'm not even sure that I agree that it's really too
> late to consider 09f08930f considering that's it's a pretty minor code
> change and the up-side to that is having reasonable defaults out of the
> box, as it were, something we have *long* been derided for.

Well, the argument against changing right now is that it would invalidate
portability testing done against beta1, which users would be justifiably
upset about.

I'm +1 for changing both of these things as soon as we branch for v14,
but I feel like it's a bit late for v13.  If we aren't feature-frozen
now, when will we be?

regards, tom lane

Re: password_encryption default

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > * Magnus Hagander (mag...@hagander.net) wrote:
> >> On Fri, May 22, 2020 at 4:13 PM Tom Lane  wrote:
> >>> Peter Eisentraut  writes:
>  We didn't get anywhere with making the default authentication method in
>  a source build anything other than trust.
> 
> > I'm +1 on moving the default for password_encryption to be
> > scram.  Even better would be changing the pg_hba.conf default, but I
> > think we still have concerns about that having problems with the
> > regression tests and the buildfarm.
> 
> As far as that last goes, we *did* get the buildfarm fixed to be all
> v11 scripts, so I thought we were ready to move forward on trying
> 09f08930f again.  It's too late to consider that for v13, but
> perhaps it'd be reasonable to change the SCRAM default now?  Not sure.

I feel like it is.  I'm not even sure that I agree that it's really too
late to consider 09f08930f considering that's it's a pretty minor code
change and the up-side to that is having reasonable defaults out of the
box, as it were, something we have *long* been derided for.

> Post-beta1 isn't the best time for such things.

It'd be good to be consistent about this between the packagers and the
source builds, imv, and we don't tend to think about that until we have
packages being built and distributed and used and that ends up being
post-beta1.  If we want that changed then we should go back to having
alphas..

In general though, I'm reasonably comfortable with changing of default
values post beta1.  I do appreciate that not everyone would agree with
that, but with all the effort that's put into getting everything working
with SCRAM, it'd be a real shame to keep md5 as the default for yet
another year and a half..

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: password_encryption default

[Tom Lane]

Stephen Frost  writes:
> * Magnus Hagander (mag...@hagander.net) wrote:
>> On Fri, May 22, 2020 at 4:13 PM Tom Lane  wrote:
>>> Peter Eisentraut  writes:
 We didn't get anywhere with making the default authentication method in
 a source build anything other than trust.

> I'm +1 on moving the default for password_encryption to be
> scram.  Even better would be changing the pg_hba.conf default, but I
> think we still have concerns about that having problems with the
> regression tests and the buildfarm.

As far as that last goes, we *did* get the buildfarm fixed to be all
v11 scripts, so I thought we were ready to move forward on trying
09f08930f again.  It's too late to consider that for v13, but
perhaps it'd be reasonable to change the SCRAM default now?  Not sure.
Post-beta1 isn't the best time for such things.

regards, tom lane

Re: About reducing EXISTS sublink

[David G. Johnston]

On Friday, May 22, 2020, Richard Guo  wrote:

> Hi hackers,
>
> For EXISTS SubLink, in some cases the subquery can be reduced to
> constant TRUE or FALSE, based on the knowledge that it's being used in
> EXISTS(). One such case is when the subquery has aggregates without
> GROUP BY or HAVING, and we know its result is exactly one row, unless
> that row is discarded by LIMIT/OFFSET. (Greenplum does this.)
>
> Is it worthwhile to add some codes for such optimization? If so, I can
> try to propose a patch
>

While the examples clearly demonstrate what you are saying they don’t
provide any motivation to do anything about it - adding aggregates and
offset to an exists subquery seems like poor query design that should be
fixed by the query writer not by spending hacker cycles optimizing it.

David J.

Re: password_encryption default

[Stephen Frost]

Greetings,

* Magnus Hagander (mag...@hagander.net) wrote:
> On Fri, May 22, 2020 at 4:13 PM Tom Lane  wrote:
> > Peter Eisentraut  writes:
> > > We didn't get anywhere with making the default authentication method in
> > > a source build anything other than trust.  But perhaps we should change
> > > the default for password_encryption to nudge people to adopt SCRAM?
> > > Right now, passwords are still hashed using MD5 by default, unless you
> > > specify scram-sha-256 using initdb -A or similar.
> >
> > I think what that was waiting on was for client libraries to become
> > SCRAM-ready.  Do we have an idea of the state of play on that side?
> >
> 
> If the summary table on the wiki at
> https://wiki.postgresql.org/wiki/List_of_drivers is to be trusted, every
> listed driver except Swift does.

Yes, Katz actually went through and worked with folks to make that
happen.  I'm +1 on moving the default for password_encryption to be
scram.  Even better would be changing the pg_hba.conf default, but I
think we still have concerns about that having problems with the
regression tests and the buildfarm.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Add explanations which are influenced by track_io_timing

[Fujii Masao]

On 2020/05/15 9:50, Atsushi Torikoshi wrote:

Thanks for reviewing!

On Wed, May 13, 2020 at 11:27 PM Fujii Masao mailto:masao.fu...@oss.nttdata.com>> wrote:

What about the attached patch based on yours?


It looks better.


Pushed. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: password_encryption default

[Magnus Hagander]

On Fri, May 22, 2020 at 4:13 PM Tom Lane  wrote:

> Peter Eisentraut  writes:
> > We didn't get anywhere with making the default authentication method in
> > a source build anything other than trust.  But perhaps we should change
> > the default for password_encryption to nudge people to adopt SCRAM?
> > Right now, passwords are still hashed using MD5 by default, unless you
> > specify scram-sha-256 using initdb -A or similar.
>
> I think what that was waiting on was for client libraries to become
> SCRAM-ready.  Do we have an idea of the state of play on that side?
>

If the summary table on the wiki at
https://wiki.postgresql.org/wiki/List_of_drivers is to be trusted, every
listed driver except Swift does.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 

Re: password_encryption default

[Tom Lane]

Peter Eisentraut  writes:
> We didn't get anywhere with making the default authentication method in 
> a source build anything other than trust.  But perhaps we should change 
> the default for password_encryption to nudge people to adopt SCRAM? 
> Right now, passwords are still hashed using MD5 by default, unless you 
> specify scram-sha-256 using initdb -A or similar.

I think what that was waiting on was for client libraries to become
SCRAM-ready.  Do we have an idea of the state of play on that side?

regards, tom lane

Re: snowball release

[Tom Lane]

Peter Eisentraut  writes:
> Snowball has made a release!  With a tag!
> I have prepared a patch to update PostgreSQL's copy.

Yeah, this was on my to-do list as well.  Thanks for doing it.

> I think some consideration could be given for including this into PG13. 
> Otherwise, I'll park it for PG14.

Meh, I think v14 at this point.  It looks more like new features
than bug fixes.

regards, tom lane

Re: Planning counters in pg_stat_statements (using pgss_store)

[Fujii Masao]

On 2020/05/22 15:10, Andy Fan wrote:



On Thu, May 21, 2020 at 3:49 PM Julien Rouhaud mailto:rjuju...@gmail.com>> wrote:

Le jeu. 21 mai 2020 à 09:17, Michael Paquier mailto:mich...@paquier.xyz>> a écrit :

On Thu, May 21, 2020 at 08:49:53AM +0200, Julien Rouhaud wrote:
 > On Tue, May 19, 2020 at 4:29 AM Andy Fan mailto:zhihui.fan1...@gmail.com>> wrote:
 >> Thanks for the excellent extension. I want to add 5 more fields to 
satisfy the
 >> following requirements.
 >>
 >> int   subplan; /* No. of subplan in this query */
 >> int   subquery; /* No. of subquery */
 >> int   joincnt; /* How many relations are joined */
 >> bool hasagg; /* if we have agg function in this query */
 >> bool hasgroup; /* has group clause */
 >
 > Most of those fields can be computed using the raw sql satements.  
Why
 > not adding functions like query_has_agg(querytext) to get the
 > information from pgss stored query text instead?

Yeah I personally find concepts related only to the query string
itself not something that needs to be tied to pg_stat_statements.
...


Indeed cte will bring additional concerns about the fields semantics. 
That's another good reason to go with external functions so you can add extra 
parameters for that if needed.


There are something more we can't get from query string easily. like:
1. view involved.   2. subquery are pulled up so there is not subquery
indeed.  3. sublink are pull-up or become as an InitPlan rather than subPlan.
4. joins are removed by remove_useless_joins.


If we can store the plan for each statement, e.g., like pg_store_plans
extension [1] does, rather than such partial information, which would
be enough for your cases?

Regards,

[1]
http://pgstoreplans.osdn.jp/pg_store_plans.html
https://github.com/ossc-db/pg_store_plans

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Dilip Kumar]

On Tue, May 19, 2020 at 5:34 PM Amit Kapila  wrote:
>
> On Fri, May 15, 2020 at 2:47 PM Dilip Kumar  wrote:
> >
> > On Tue, May 12, 2020 at 4:39 PM Amit Kapila  wrote:
> > >
> >
> > > 4.
> > > +static void
> > > +stream_start_cb_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn)
> > > +{
> > > + LogicalDecodingContext *ctx = cache->private_data;
> > > + LogicalErrorCallbackState state;
> > > + ErrorContextCallback errcallback;
> > > +
> > > + Assert(!ctx->fast_forward);
> > > +
> > > + /* We're only supposed to call this when streaming is supported. */
> > > + Assert(ctx->streaming);
> > > +
> > > + /* Push callback + info on the error context stack */
> > > + state.ctx = ctx;
> > > + state.callback_name = "stream_start";
> > > + /* state.report_location = apply_lsn; */
> > >
> > > Why can't we supply the report_location here?  I think here we need to
> > > report txn->first_lsn if this is the very first stream and
> > > txn->final_lsn if it is any consecutive one.
> >
> > Done
> >
>
> Now after your change in stream_start_cb_wrapper, we assign
> report_location as first_lsn passed as input to function but
> write_location is still txn->first_lsn.  Shouldn't we assing passed in
> first_lsn to write_location?  It seems assigning txn->first_lsn won't
> be correct for streams other than first-one.

Done

>
> > > 5.
> > > +static void
> > > +stream_stop_cb_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn)
> > > +{
> > > + LogicalDecodingContext *ctx = cache->private_data;
> > > + LogicalErrorCallbackState state;
> > > + ErrorContextCallback errcallback;
> > > +
> > > + Assert(!ctx->fast_forward);
> > > +
> > > + /* We're only supposed to call this when streaming is supported. */
> > > + Assert(ctx->streaming);
> > > +
> > > + /* Push callback + info on the error context stack */
> > > + state.ctx = ctx;
> > > + state.callback_name = "stream_stop";
> > > + /* state.report_location = apply_lsn; */
> > >
> > > Can't we report txn->final_lsn here
> >
> > We are already setting this to the  txn->final_ls in 0006 patch, but I
> > have moved it into this patch now.
> >
>
> Similar to previous point, here also, I think we need to assign report
> and write location as last_lsn passed to this API.

Done

> >
> > > v20-0005-Implement-streaming-mode-in-ReorderBuffer
> > > -
> > > 10.
> > > Theoretically, we could get rid of the k-way merge, and append the
> > > changes to the toplevel xact directly (and remember the position
> > > in the list in case the subxact gets aborted later).
> > >
> > > I don't think this part of the commit message is correct as we
> > > sometimes need to spill even during streaming.  Please check the
> > > entire commit message and update according to the latest
> > > implementation.
> >
> > Done
> >
>
> You seem to forgot about removing the other part of message ("This
> adds a second iterator for the streaming case" which is not
> relavant now.

Done


> > > 11.
> > > - * HeapTupleSatisfiesHistoricMVCC.
> > > + * tqual.c's HeapTupleSatisfiesHistoricMVCC.
> > > + *
> > > + * We do build the hash table even if there are no CIDs. That's
> > > + * because when streaming in-progress transactions we may run into
> > > + * tuples with the CID before actually decoding them. Think e.g. about
> > > + * INSERT followed by TRUNCATE, where the TRUNCATE may not be decoded
> > > + * yet when applying the INSERT. So we build a hash table so that
> > > + * ResolveCminCmaxDuringDecoding does not segfault in this case.
> > > + *
> > > + * XXX We might limit this behavior to streaming mode, and just bail
> > > + * out when decoding transaction at commit time (at which point it's
> > > + * guaranteed to see all CIDs).
> > >   */
> > >  static void
> > >  ReorderBufferBuildTupleCidHash(ReorderBuffer *rb, ReorderBufferTXN *txn)
> > > @@ -1350,9 +1498,6 @@ ReorderBufferBuildTupleCidHash(ReorderBuffer
> > > *rb, ReorderBufferTXN *txn)
> > >   dlist_iter iter;
> > >   HASHCTL hash_ctl;
> > >
> > > - if (!rbtxn_has_catalog_changes(txn) || dlist_is_empty(&txn->tuplecids))
> > > - return;
> > > -
> > >
> > > I don't understand this change.  Why would "INSERT followed by
> > > TRUNCATE" could lead to a tuple which can come for decode before its
> > > CID?  The patch has made changes based on this assumption in
> > > HeapTupleSatisfiesHistoricMVCC which appears to be very risky as the
> > > behavior could be dependent on whether we are streaming the changes
> > > for in-progress xact or at the commit of a transaction.  We might want
> > > to generate a test to once validate this behavior.
> > >
> > > Also, the comment refers to tqual.c which is wrong as this API is now
> > > in heapam_visibility.c.
> >
> > Done.
> >
>
> + * INSERT.  So in such cases we assume the CIDs is from the future command
> + * and return as unresolve.
> + */
> + if (tuplecid_data == NULL)
> + return false;
> +
>
> Here lets reword the last line of comment as ".  So in

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Dilip Kumar]

On Tue, May 19, 2020 at 6:01 PM Amit Kapila  wrote:
>
> On Fri, May 15, 2020 at 2:48 PM Dilip Kumar  wrote:
> >
> > On Wed, May 13, 2020 at 4:50 PM Amit Kapila  wrote:
> > >
> >
> > > 3.
> > > And, during catalog scan we can check the status of the xid and
> > > + * if it is aborted we will report a specific error that we can ignore.  
> > > We
> > > + * might have already streamed some of the changes for the aborted
> > > + * (sub)transaction, but that is fine because when we decode the abort 
> > > we will
> > > + * stream abort message to truncate the changes in the subscriber.
> > > + */
> > > +static inline void
> > > +SetupCheckXidLive(TransactionId xid)
> > >
> > > In the above comment, I don't think it is right to say that we ignore
> > > the error raised due to the aborted transaction.  We need to say that
> > > we discard the already streamed changes on such an error.
> >
> > Done.
> >
>
> In the same comment, there is typo (/messageto/message to).

Done

> > > 4.
> > > +static inline void
> > > +SetupCheckXidLive(TransactionId xid)
> > > +{
> > >   /*
> > > - * If this transaction has no snapshot, it didn't make any changes to the
> > > - * database, so there's nothing to decode.  Note that
> > > - * ReorderBufferCommitChild will have transferred any snapshots from
> > > - * subtransactions if there were any.
> > > + * setup CheckXidAlive if it's not committed yet. We don't check if the 
> > > xid
> > > + * aborted. That will happen during catalog access.  Also reset the
> > > + * sysbegin_called flag.
> > >   */
> > > - if (txn->base_snapshot == NULL)
> > > + if (!TransactionIdDidCommit(xid))
> > >   {
> > > - Assert(txn->ninvalidations == 0);
> > > - ReorderBufferCleanupTXN(rb, txn);
> > > - return;
> > > + CheckXidAlive = xid;
> > > + bsysscan = false;
> > >   }
> > >
> > > I think this function is inline as it needs to be called for each
> > > change. If that is the case and otherwise also, isn't it better that
> > > we check if passed xid is the same as CheckXidAlive before checking
> > > TransactionIdDidCommit as TransactionIdDidCommit can be costly and
> > > calling it for each change might not be a good idea?
> >
> > Done,  Also I think it is good the check the TransactionIdIsInProgress
> > instead of !TransactionIdDidCommit.  I have changed that as well.
> >
>
> What if it is aborted just before this check?  I think the decode API
> won't be able to detect that and sys* API won't care to check because
> CheckXidAlive won't be set for that case.

Yeah, that's the problem,  I think it should be TransactionIdDidCommit only.

> > > 5.
> > > setup CheckXidAlive if it's not committed yet. We don't check if the xid
> > > + * aborted. That will happen during catalog access.  Also reset the
> > > + * sysbegin_called flag.
> > >
> > > /if the xid aborted/if the xid is aborted.  missing comma after Also.
> >
> > Done
> >
>
> You forgot to change as per the second part of the comment (missing
> comma after Also).

Done


> > > 8.
> > > @@ -1588,8 +1766,6 @@ ReorderBufferCommit(ReorderBuffer *rb, 
> > > TransactionId xid,
> > >   * use as a normal record. It'll be cleaned up at the end
> > >   * of INSERT processing.
> > >   */
> > > - if (specinsert == NULL)
> > > - elog(ERROR, "invalid ordering of speculative insertion changes");
> > >
> > > You have removed this check but all other handling of specinsert is
> > > same as far as this patch is concerned.  Why so?
> >
> > Seems like a merge issue, or the leftover from the old design of the
> > toast handling where we were streaming with the partial tuple.
> > fixed now.
> >
> > > 9.
> > > @@ -1676,8 +1860,6 @@ ReorderBufferCommit(ReorderBuffer *rb, 
> > > TransactionId xid,
> > >   * freed/reused while restoring spooled data from
> > >   * disk.
> > >   */
> > > - Assert(change->data.tp.newtuple != NULL);
> > > -
> > >   dlist_delete(&change->node);
> > >
> > > Why is this Assert removed?
> >
> > Same cause as above so fixed.
> >
> > > 10.
> > > @@ -1753,7 +1935,15 @@ ReorderBufferCommit(ReorderBuffer *rb, 
> > > TransactionId xid,
> > >   relations[nrelations++] = relation;
> > >   }
> > >
> > > - rb->apply_truncate(rb, txn, nrelations, relations, change);
> > > + if (streaming)
> > > + {
> > > + rb->stream_truncate(rb, txn, nrelations, relations, change);
> > > +
> > > + /* Remember that we have sent some data. */
> > > + change->txn->any_data_sent = true;
> > > + }
> > > + else
> > > + rb->apply_truncate(rb, txn, nrelations, relations, change);
> > >
> > > Can we encapsulate this in a separate function like
> > > ReorderBufferApplyTruncate or something like that?  Basically, rather
> > > than having streaming check in this function, lets do it in some other
> > > internal function.  And we can likewise do it for all the streaming
> > > checks in this function or at least whereever it is feasible.  That
> > > will make this function look clean.
> >
> > Done for truncate and change.  I think we can create a few more such
> > functions for
> > start

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Dilip Kumar]

On Tue, May 19, 2020 at 4:33 PM Amit Kapila  wrote:
>
> On Tue, May 19, 2020 at 3:31 PM Dilip Kumar  wrote:
> >
> > On Tue, May 19, 2020 at 2:34 PM Amit Kapila  wrote:
> > >
> > > On Mon, May 18, 2020 at 5:57 PM Amit Kapila  
> > > wrote:
> > > >
> > > >
> > > > 3.
> > > > + /*
> > > > + * If streaming is enable and we have serialized this transaction 
> > > > because
> > > > + * it had incomplete tuple.  So if now we have got the complete tuple 
> > > > we
> > > > + * can stream it.
> > > > + */
> > > > + if (ReorderBufferCanStream(rb) && can_stream && 
> > > > rbtxn_is_serialized(toptxn)
> > > > + && !(rbtxn_has_toast_insert(txn)) && !(rbtxn_has_spec_insert(txn)))
> > > > + {
> > > >
> > > > This comment is just saying what you are doing in the if-check.  I
> > > > think you need to explain the rationale behind it. I don't like the
> > > > variable name 'can_stream' because it matches ReorderBufferCanStream
> > > > whereas it is for a different purpose, how about naming it as
> > > > 'change_complete' or something like that.  The check has many
> > > > conditions, can we move it to a separate function to make the code
> > > > here look clean?
> > > >
> > >
> > > Do we really need this?  Immediately after this check, we are calling
> > > ReorderBufferCheckMemoryLimit which will anyway stream the changes if
> > > required.
> >
> > Actually, ReorderBufferCheckMemoryLimit is only meant for checking
> > whether we need to stream the changes due to the memory limit.  But
> > suppose when memory limit exceeds that time we could not stream the
> > transaction because there was only incomplete toast insert so we
> > serialized.  Now,  when we get the tuple which makes the changes
> > complete but now it is not crossing the memory limit as changes were
> > already serialized.  So I am not sure whether it is a good idea to
> > stream the transaction as soon as we get the complete changes or we
> > shall wait till next time memory limit exceed and that time we select
> > the suitable candidate.
> >
>
> I think it is better to wait till next time we exceed the memory threshold.

Okay, done this way.


> >  Ideally, we were are in streaming more and
> > the transaction is serialized means it was already a candidate for
> > streaming but could not stream due to the incomplete changes so
> > shouldn't we stream it immediately as soon as its changes are complete
> > even though now we are in memory limit.
> >
>
> The only time we need to stream or spill is when we exceed memory
> threshold.  In the above case, it is possible that next time there is
> some other candidate transaction that we can stream.
>
> > >
> > > Another comments on v20-0010-Bugfix-handling-of-incomplete-toast-tuple:
> > >
> > > + else if (rbtxn_has_toast_insert(txn) &&
> > > + ChangeIsInsertOrUpdate(change->action))
> > > + {
> > > + toptxn->txn_flags &= ~RBTXN_HAS_TOAST_INSERT;
> > > + can_stream = true;
> > > + }
> > > ..
> > > +#define ChangeIsInsertOrUpdate(action) \
> > > + (((action) == REORDER_BUFFER_CHANGE_INSERT) || \
> > > + ((action) == REORDER_BUFFER_CHANGE_UPDATE) || \
> > > + ((action) == REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT))
> > >
> > > How can we clear the RBTXN_HAS_TOAST_INSERT flag on
> > > REORDER_BUFFER_CHANGE_INTERNAL_SPEC_INSERT action?
> >
> > Partial toast insert means we have inserted in the toast but not in
> > the main table.  So even if it is spec insert we can form the complete
> > tuple, however, we can still not stream it because we haven't got
> > spec_confirm but for that, we are marking another flag.  So if the
> > insert is aspect insert the toast insert will also be spec insert and
> > as part of that toast, spec inserts we are marking partial tuple so
> > cleaning that flag should happen when the spec insert is done for the
> > main table right?
> >
>
> Sounds reasonable.

ok




--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Dilip Kumar]

On Mon, May 18, 2020 at 4:10 PM Amit Kapila  wrote:
>
> On Sun, May 17, 2020 at 12:41 PM Dilip Kumar  wrote:
> >
> > On Fri, May 15, 2020 at 4:04 PM Amit Kapila  wrote:
> > >
> > >
> > > Review comments:
> > > --
> > > 1.
> > > @@ -1762,10 +1952,16 @@ ReorderBufferCommit(ReorderBuffer *rb,
> > > TransactionId xid,
> > >   }
> > >
> > >   case REORDER_BUFFER_CHANGE_MESSAGE:
> > > - rb->message(rb, txn, change->lsn, true,
> > > - change->data.msg.prefix,
> > > - change->data.msg.message_size,
> > > - change->data.msg.message);
> > > + if (streaming)
> > > + rb->stream_message(rb, txn, change->lsn, true,
> > > +change->data.msg.prefix,
> > > +change->data.msg.message_size,
> > > +change->data.msg.message);
> > > + else
> > > + rb->message(rb, txn, change->lsn, true,
> > > +change->data.msg.prefix,
> > > +change->data.msg.message_size,
> > > +change->data.msg.message);
> > >
> > > Don't we need to set any_data_sent flag while streaming messages as we
> > > do for other types of changes?
> >
> > I think any_data_sent, was added to avoid sending abort to the
> > subscriber if we haven't sent any data,  but this is not complete as
> > the output plugin can also take the decision not to send.  So I think
> > this should not be done as part of this patch and can be done
> > separately.  I think there is already a thread for handling the
> > same[1]
> >
>
> Hmm, but prior to this patch, we never use to send (empty) aborts but
> now that will be possible. It is probably okay to deal that with
> another patch mentioned by you but I felt at least any_data_sent will
> work for some cases.  OTOH, it appears to be half-baked solution, so
> we should probably refrain from adding it.  BTW, how do the pgoutput
> plugin deal with it? I see that apply_handle_stream_abort will
> unconditionally try to unlink the file and it will probably fail.
> Have you tested this scenario after your latest changes?

Yeah, I see, I think this is a problem,  but this exists without my
latest change as well, if pgoutput ignore some changes because it is
not published then we will see a similar error.  Shall we handle the
ENOENT error case from unlink?  I think the best idea is that we shall
track the empty transaction.

> > > 4.
> > > In ReorderBufferProcessTXN(), the patch is calling stream_stop in both
> > > the try and catch block.  If there is an error after calling it in a
> > > try block, we might call it again via catch.  I think that will lead
> > > to sending a stop message twice.  Won't that be a problem?  See the
> > > usage of iterstate in the catch block, we have made it safe from a
> > > similar problem.
> >
> > IMHO, we don't need that, because we only call stream_stop in the
> > catch block if the error type is ERRCODE_TRANSACTION_ROLLBACK.  So if
> > in TRY block we have already stopped the stream then we should not get
> > that error.  I have added the comments for the same.
> >
>
> I am still slightly nervous about it as I don't see any solid
> guarantee for the same.  You are right as the code stands today but
> due to any code that gets added in the future, it might not remain
> true. I feel it is better to have an Assert here to ensure that
> stream_stop won't be called the second time.  I don't see any good way
> of doing it other than by maintaining flag or some state but I think
> it will be good to ensure this.

Done

> > > 6.
> > > PG_CATCH();
> > >   {
> > > + MemoryContext ecxt = MemoryContextSwitchTo(ccxt);
> > > + ErrorData  *errdata = CopyErrorData();
> > >
> > > I don't understand the usage of memory context in this part of the
> > > code.  Basically, you are switching to CurrentMemoryContext here, do
> > > some error handling and then again reset back to some random context
> > > before rethrowing the error.  If there is some purpose for it, then it
> > > might be better if you can write a few comments to explain the same.
> >
> > Basically, the ccxt is the CurrentMemoryContext when we started the
> > streaming and ecxt it the context when we catch the error.  So
> > ideally, before this change, it will rethrow in the context when we
> > catch the error i.e. ecxt.  So what we are trying to do is put it back
> > to normal context (ccxt) and copy the error data in the normal
> > context.  And, if we are not handling it gracefully then put it back
> > to the context it was in, and rethrow.
> >
>
> Okay, but when errorcode is *not* ERRCODE_TRANSACTION_ROLLBACK, don't
> we need to clean up the reorderbuffer by calling
> ReorderBufferCleanupTXN?  If so, then you can try to combine it with
> the not-streaming else loop.

Done


> > > 8.
> > > @@ -2295,6 +2677,13 @@ ReorderBufferXidSetCatalogChanges(ReorderBuffer
> > > *rb, TransactionId xid,
> > >   txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
> > >
> > >   txn->txn_flags |= RBTXN_HAS_CATALOG_CHANGES;
> > > +
> > > + /*
> > > + * TOCHECK: Mark toplevel transaction as having catalog changes too
> > > +

password_encryption default

[Peter Eisentraut]

We didn't get anywhere with making the default authentication method in 
a source build anything other than trust.  But perhaps we should change 
the default for password_encryption to nudge people to adopt SCRAM? 
Right now, passwords are still hashed using MD5 by default, unless you 
specify scram-sha-256 using initdb -A or similar.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

snowball release

[Peter Eisentraut]

Snowball has made a release!  With a tag!

I have prepared a patch to update PostgreSQL's copy.  (not attached 
here, 566230 bytes, but see 
https://github.com/petere/postgresql/commit/52a6133b58c77ada4210a96e5155cbe4da5e5583)


Since we last updated our copy from their commit date 2019-06-24 and the 
release is from 2019-10-02, the changes are pretty small and mostly 
reformatting.  But there are three new stemmers: Basque, Catalan, Hindi.


I think some consideration could be given for including this into PG13. 
Otherwise, I'll park it for PG14.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: SEARCH and CYCLE clauses

[Peter Eisentraut]

On 2020-05-22 12:40, Vik Fearing wrote:

If you specify null, then the cycle check expression will always fail,
so it will abort after the first row.  (We should perhaps prohibit
specifying null, but see below.)


I would rather make the cycle check expression work with null.


It works correctly AFAICT.  What is your expectation?


This is something we need to think about.  If we want to check at parse
time whether the two values are not the same (and perhaps not null),
then we either need to restrict the generality of what we can specify,
or we need to be prepared to do full expression evaluation in the
parser.  A simple and practical way might be to only allow string and
boolean literal.  I don't have a strong opinion here.



I'm with Pavel on this one.  Why does it have to be a parse-time error?
  Also, I regularly see people write functions as sort of pauper's
variables, but a function call isn't allowed here.


If not parse-time error, at what time do you want to check it?


As an improvement over the spec, I think the vast majority of people
will be using simple true/false values.  Can we make that optional?

 CYCLE f, t SET is_cycle USING path

would be the same as

 CYCLE f, t SET is_cycle TO true DEFAULT false USING path


I was also considering that.  It would be an easy change to make.

(Apparently, in DB2 you can omit the USING path clause.  Not sure how to 
make that work, however.)


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Getting ERROR with FOR UPDATE/SHARE for partitioned table.

[amul sul]

On Fri, May 22, 2020 at 5:00 PM Rajkumar Raghuwanshi <
rajkumar.raghuwan...@enterprisedb.com> wrote:

> Hi All,
>
> I am getting ERROR when using the "FOR UPDATE" clause for the partitioned
> table. below is a reproducible test case for the same.
>
> CREATE TABLE tbl (c1 INT,c2 TEXT) PARTITION BY LIST (c1);
> CREATE TABLE tbl_null PARTITION OF tbl FOR VALUES IN (NULL);
> CREATE TABLE tbl_1 PARTITION OF tbl FOR VALUES IN (1,2,3);
>
> INSERT INTO tbl SELECT i,i FROM generate_series(1,3) i;
>
> CREATE OR REPLACE FUNCTION func(i int) RETURNS int
> AS $$
> DECLARE
>  v_var tbl%ROWTYPE;
>  cur CURSOR IS SELECT * FROM tbl WHERE c1< 5 FOR UPDATE;
> BEGIN
>  OPEN cur;
>  LOOP
>   FETCH cur INTO v_var;
>   EXIT WHEN NOT FOUND;
>   UPDATE tbl SET c2='aa' WHERE CURRENT OF cur;
>  END LOOP;
>  CLOSE cur;
>  RETURN 10;
> END;
> $$ LANGUAGE PLPGSQL;
>
> SELECT func(10);
>

I tried similar things on inherit partitioning as follow and that looks
fine:

DROP TABLE tbl;
CREATE TABLE tbl (c1 INT,c2 TEXT);
CREATE TABLE tbl_null(check (c1 is NULL)) INHERITS (tbl);
CREATE TABLE tbl_1 (check (c1 > 0 and c1 < 4)) INHERITS (tbl);
INSERT INTO tbl_1 VALUES(generate_series(1,3));

postgres=# SELECT func(10);
 func
--
   10
(1 row)

On looking further for declarative partition, I found that issue happens
only if
the partitioning pruning enabled, see this:

-- Execute on original set of test case.
postgres=# ALTER FUNCTION func SET enable_partition_pruning to off;
ALTER FUNCTION

postgres=# SELECT func(10);
 func
--
   10
(1 row)

I think we need some indication in execCurrentOf() to skip error if the
relation
is pruned.  Something like that we already doing for inheriting
partitioning,
see following comment execCurrentOf():

/*
 * This table didn't produce the cursor's current row; some other
 * inheritance child of the same parent must have.  Signal caller to
 * do nothing on this table.
 */

Regards,
Amul

Getting ERROR with FOR UPDATE/SHARE for partitioned table.

[Rajkumar Raghuwanshi]

Hi All,

I am getting ERROR when using the "FOR UPDATE" clause for the partitioned
table. below is a reproducible test case for the same.

CREATE TABLE tbl (c1 INT,c2 TEXT) PARTITION BY LIST (c1);
CREATE TABLE tbl_null PARTITION OF tbl FOR VALUES IN (NULL);
CREATE TABLE tbl_1 PARTITION OF tbl FOR VALUES IN (1,2,3);

INSERT INTO tbl SELECT i,i FROM generate_series(1,3) i;

CREATE OR REPLACE FUNCTION func(i int) RETURNS int
AS $$
DECLARE
 v_var tbl%ROWTYPE;
 cur CURSOR IS SELECT * FROM tbl WHERE c1< 5 FOR UPDATE;
BEGIN
 OPEN cur;
 LOOP
  FETCH cur INTO v_var;
  EXIT WHEN NOT FOUND;
  UPDATE tbl SET c2='aa' WHERE CURRENT OF cur;
 END LOOP;
 CLOSE cur;
 RETURN 10;
END;
$$ LANGUAGE PLPGSQL;

SELECT func(10);

postgres=# SELECT func(10);
ERROR:  cursor "cur" does not have a FOR UPDATE/SHARE reference to table
"tbl_null"
CONTEXT:  SQL statement "UPDATE tbl SET c2='aa' WHERE CURRENT OF cur"
PL/pgSQL function func(integer) line 10 at SQL statement

Thanks & Regards,
Rajkumar Raghuwanshi

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Amit Kapila]

On Fri, May 22, 2020 at 11:54 AM Amit Kapila  wrote:
>
> v22-0006-Add-support-for-streaming-to-built-in-replicatio
> 
>
Few more comments on v22-0006 patch:

1.
+stream_cleanup_files(Oid subid, TransactionId xid, bool missing_ok)
+{
+ int i;
+ char path[MAXPGPATH];
+ bool found = false;
+
+ subxact_filename(path, subid, xid);
+
+ if ((unlink(path) < 0) && (errno != ENOENT) && !missing_ok)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));

Here, we have unlinked the files containing information of subxacts
but don't we need to free the corresponding memory (memory for
subxacts) as well?

2.
apply_handle_stream_abort()
{
..
+ subxact_filename(path, MyLogicalRepWorker->subid, xid);
+
+ if (unlink(path) < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not remove file \"%s\": %m", path)));
+
+ return;
..
}

Like the previous comment, it seems here also we need to free subxacts
memory and additionally we forgot to adjust the xids array as well.

3.
apply_handle_stream_abort()
{
..
+ /* XXX optimize the search by bsearch on sorted data */
+ for (i = nsubxacts; i > 0; i--)
+ {
+ if (subxacts[i - 1].xid == subxid)
+ {
+ subidx = (i - 1);
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ return;
..
}

Is it possible that we didn't find the xid in subxacts array?  If so,
I think we should mention the same in comments, otherwise, we should
have an assert for found.

4.
apply_handle_stream_abort()
{
..
+ changes_filename(path, MyLogicalRepWorker->subid, xid);
+
+ if (truncate(path, subxacts[subidx].offset))
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not truncate file \"%s\": %m", path)));
..
}

Will truncate works on Windows?  I see in the code we ftruncate which
is defined as chsize in win32.h and win32_port.h.  I have not tested
this so I am not very sure about this.  I got a below warning when I
tried to compile this code on Windows.  I think it is better to
ftruncate as it is used at other places in the code as well.

worker.c(798): warning C4013: 'truncate' undefined; assuming extern
returning int

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: SEARCH and CYCLE clauses

[Vik Fearing]

On 5/22/20 11:24 AM, Peter Eisentraut wrote:
> On 2020-05-20 21:28, Vik Fearing wrote:
>> 1)
>> There are some smart quotes in the comments that should be converted to
>> single quotes.
> 
> ok, fixing that
> 
>> 2)
>> This query is an infinite loop, as expected:
>>
>>    with recursive a as (select 1 as b union all select b from a)
>>    table a;
>>
>> But it becomes an error when you add a cycle clause to it:
>>
>>    with recursive a as (select 1 as b union all table a)
>>  cycle b set c to true default false using p
>>    table a;
>>
>>    ERROR:  each UNION query must have the same number of columns
> 
> table a expands to select * from a, and if you have a cycle clause, then
> a has three columns, but the other branch of the union only has one, so
> that won't work anymore, will it?

It seems there was a copy/paste error here.  The first query should have
been the same as the second but without the cycle clause.

It seems strange to me that adding a  would
break a previously working query.  I would rather see the * expanded
before adding the new columns.  This is a user's opinion, I don't know
how hard that would be to implement.

>> 3)
>> If I take the same infinite loop query but replace the TABLE syntax with
>> a SELECT and add a cycle clause, it's not an infinite loop anymore.
>>
>>    with recursive a as (select 1 as b union all select b from a)
>>  cycle b set c to true default false using p
>>    table a;
>>
>>   b | c | p
>> ---+---+---
>>   1 | f | {(1)}
>>   1 | t | {(1),(1)}
>> (2 rows)
>>
>> Why does it stop?  It should still be an infinite loop.
> 
> If you specify the cycle clause, then the processing will stop if it
> sees the same row more than once, which it did here.

Yes, this was a misplaced expectation on my part.

>> 4)
>> If I use NULL instead of false, I only get one row back.
>>
>>    with recursive a as (select 1 as b union all select b from a)
>>  cycle b set c to true default false using p
>>    table a;
>>
>>   b | c |   p
>> ---+---+---
>>   1 |   | {(1)}
>> (1 row)
> 
> If you specify null, then the cycle check expression will always fail,
> so it will abort after the first row.  (We should perhaps prohibit
> specifying null, but see below.)

I would rather make the cycle check expression work with null.

>> 5)
>> I can set both states to the same value.
>>
>>    with recursive a as (select 1 as b union all select b from a)
>>  cycle b set c to true default true using p
>>    table a;
> 
>> This is a direct violation of 7.18 SR 2.b.ii.3 as well as common sense.
>>   BTW, I applaud your decision to violate the other part of that rule and
>> allowing any data type here.
>>
>>
>> 5)
>> The same rule as above says that the value and the default value must be
>> literals but not everything that a human might consider a literal is
>> accepted.  In particular:
>>
>>    with recursive a as (select 1 as b union all select b from a)
>>  cycle b set c to 1 default -1 using p
>>    table a;
>>
>>    ERROR:  syntax error at or near "-"
>>
>> Can we just accept a full a_expr here instead of AexprConst?  Both
>> DEFAULT and USING are fully reserved keywords.
> 
> This is something we need to think about.  If we want to check at parse
> time whether the two values are not the same (and perhaps not null),
> then we either need to restrict the generality of what we can specify,
> or we need to be prepared to do full expression evaluation in the
> parser.  A simple and practical way might be to only allow string and
> boolean literal.  I don't have a strong opinion here.


I'm with Pavel on this one.  Why does it have to be a parse-time error?
 Also, I regularly see people write functions as sort of pauper's
variables, but a function call isn't allowed here.



Another bug I found.  If I try to do your regression test as an
autonomous query, I get this:

with recursive

graph (f, t, label) as (
values (1, 2, 'arc 1 -> 2'),
   (1, 3, 'arc 1 -> 3'),
   (2, 3, 'arc 2 -> 3'),
   (1, 4, 'arc 1 -> 4'),
   (4, 5, 'arc 4 -> 5'),
   (5, 1, 'arc 5 -> 1')
),

search_graph (f, t, label) as (
select * from graph g
union all
select g.*
from graph g, search_graph sg
where g.f = sg.t
)
cycle f, t set is_cycle to true default false using path

select * from search_graph;

ERROR:  could not find CTE "graph"



As an improvement over the spec, I think the vast majority of people
will be using simple true/false values.  Can we make that optional?

CYCLE f, t SET is_cycle USING path

would be the same as

CYCLE f, t SET is_cycle TO true DEFAULT false USING path
-- 
Vik Fearing

Re: SEARCH and CYCLE clauses

[Pavel Stehule]

pá 22. 5. 2020 v 11:25 odesílatel Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> napsal:

> On 2020-05-20 21:28, Vik Fearing wrote:
> > 1)
> > There are some smart quotes in the comments that should be converted to
> > single quotes.
>
> ok, fixing that
>
> > 2)
> > This query is an infinite loop, as expected:
> >
> >with recursive a as (select 1 as b union all select b from a)
> >table a;
> >
> > But it becomes an error when you add a cycle clause to it:
> >
> >with recursive a as (select 1 as b union all table a)
> >  cycle b set c to true default false using p
> >table a;
> >
> >ERROR:  each UNION query must have the same number of columns
>
> table a expands to select * from a, and if you have a cycle clause, then
> a has three columns, but the other branch of the union only has one, so
> that won't work anymore, will it?
>
> > 3)
> > If I take the same infinite loop query but replace the TABLE syntax with
> > a SELECT and add a cycle clause, it's not an infinite loop anymore.
> >
> >with recursive a as (select 1 as b union all select b from a)
> >  cycle b set c to true default false using p
> >table a;
> >
> >   b | c | p
> > ---+---+---
> >   1 | f | {(1)}
> >   1 | t | {(1),(1)}
> > (2 rows)
> >
> > Why does it stop?  It should still be an infinite loop.
>
> If you specify the cycle clause, then the processing will stop if it
> sees the same row more than once, which it did here.
>
> > 4)
> > If I use NULL instead of false, I only get one row back.
> >
> >with recursive a as (select 1 as b union all select b from a)
> >  cycle b set c to true default false using p
> >table a;
> >
> >   b | c |   p
> > ---+---+---
> >   1 |   | {(1)}
> > (1 row)
>
> If you specify null, then the cycle check expression will always fail,
> so it will abort after the first row.  (We should perhaps prohibit
> specifying null, but see below.)
>
> > 5)
> > I can set both states to the same value.
> >
> >with recursive a as (select 1 as b union all select b from a)
> >  cycle b set c to true default true using p
> >table a;
>
> > This is a direct violation of 7.18 SR 2.b.ii.3 as well as common sense.
> >   BTW, I applaud your decision to violate the other part of that rule and
> > allowing any data type here.
> >
> >
> > 5)
> > The same rule as above says that the value and the default value must be
> > literals but not everything that a human might consider a literal is
> > accepted.  In particular:
> >
> >with recursive a as (select 1 as b union all select b from a)
> >  cycle b set c to 1 default -1 using p
> >table a;
> >
> >ERROR:  syntax error at or near "-"
> >
> > Can we just accept a full a_expr here instead of AexprConst?  Both
> > DEFAULT and USING are fully reserved keywords.
>
> This is something we need to think about.  If we want to check at parse
> time whether the two values are not the same (and perhaps not null),
> then we either need to restrict the generality of what we can specify,
> or we need to be prepared to do full expression evaluation in the
> parser.  A simple and practical way might be to only allow string and
> boolean literal.  I don't have a strong opinion here.
>

if you check it in parse time, then you disallow parametrization there.

Is any reason to do this check in parse time?


> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
>
>

Re: pgindent vs dtrace on macos

[Peter Eisentraut]

On 2020-05-20 15:56, Tom Lane wrote:

On a closely related point: I was confused for awhile on Monday
afternoon, wondering why the built tarballs didn't match my local
tree.  I eventually realized that when I'd run pgindent on Saturday,
it had reformatted some generated files such as
src/bin/psql/sql_help.h, causing those not to match the freshly-made
ones in the tarball.  I wonder if we should make an effort to ensure
that our generated .h and .c files always satisfy pgindent.


We should generally try to do that, if only so that they don't appear 
weird and random when looking at them.


I think in the past it would have been very difficult for a generation 
script to emulate pgindent's weird un-indentation logic on trailing 
lines, but that shouldn't be a problem anymore.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: SEARCH and CYCLE clauses

[Peter Eisentraut]

On 2020-05-20 21:28, Vik Fearing wrote:

1)
There are some smart quotes in the comments that should be converted to
single quotes.


ok, fixing that


2)
This query is an infinite loop, as expected:

   with recursive a as (select 1 as b union all select b from a)
   table a;

But it becomes an error when you add a cycle clause to it:

   with recursive a as (select 1 as b union all table a)
 cycle b set c to true default false using p
   table a;

   ERROR:  each UNION query must have the same number of columns


table a expands to select * from a, and if you have a cycle clause, then 
a has three columns, but the other branch of the union only has one, so 
that won't work anymore, will it?



3)
If I take the same infinite loop query but replace the TABLE syntax with
a SELECT and add a cycle clause, it's not an infinite loop anymore.

   with recursive a as (select 1 as b union all select b from a)
 cycle b set c to true default false using p
   table a;

  b | c | p
---+---+---
  1 | f | {(1)}
  1 | t | {(1),(1)}
(2 rows)

Why does it stop?  It should still be an infinite loop.


If you specify the cycle clause, then the processing will stop if it 
sees the same row more than once, which it did here.



4)
If I use NULL instead of false, I only get one row back.

   with recursive a as (select 1 as b union all select b from a)
 cycle b set c to true default false using p
   table a;

  b | c |   p
---+---+---
  1 |   | {(1)}
(1 row)


If you specify null, then the cycle check expression will always fail, 
so it will abort after the first row.  (We should perhaps prohibit 
specifying null, but see below.)



5)
I can set both states to the same value.

   with recursive a as (select 1 as b union all select b from a)
 cycle b set c to true default true using p
   table a;



This is a direct violation of 7.18 SR 2.b.ii.3 as well as common sense.
  BTW, I applaud your decision to violate the other part of that rule and
allowing any data type here.


5)
The same rule as above says that the value and the default value must be
literals but not everything that a human might consider a literal is
accepted.  In particular:

   with recursive a as (select 1 as b union all select b from a)
 cycle b set c to 1 default -1 using p
   table a;

   ERROR:  syntax error at or near "-"

Can we just accept a full a_expr here instead of AexprConst?  Both
DEFAULT and USING are fully reserved keywords.


This is something we need to think about.  If we want to check at parse 
time whether the two values are not the same (and perhaps not null), 
then we either need to restrict the generality of what we can specify, 
or we need to be prepared to do full expression evaluation in the 
parser.  A simple and practical way might be to only allow string and 
boolean literal.  I don't have a strong opinion here.


--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: some grammar refactoring

[Peter Eisentraut]

On 2020-05-19 08:43, Peter Eisentraut wrote:

Here is a series of patches to do some refactoring in the grammar around
the commands COMMENT, DROP, SECURITY LABEL, and ALTER EXTENSION ...
ADD/DROP.  In the grammar, these commands (with some exceptions)
basically just take a reference to an object and later look it up in C
code.  Some of that was already generalized individually for each
command (drop_type_any_name, drop_type_name, etc.).  This patch combines
it into common lists for all these commands.


While most of this patch set makes no behavior changes by design, I 
should point out this little change hidden in the middle:


Remove deprecated syntax from CREATE/DROP LANGUAGE

Remove the option to specify the language name as a single-quoted
string.  This has been obsolete since ee8ed85da3b.  Removing it allows
better grammar refactoring.

The syntax of the CREATE FUNCTION LANGUAGE clause is not changed.

(ee8ed85da3b is in PG 7.2.)

I expect this to be uncontroversial, but it should be pointed out.

--
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Optimizer docs typos

[Richard Guo]

On Fri, May 22, 2020 at 2:53 PM Etsuro Fujita 
wrote:

>
> Done.
>

Thanks!

Thanks
Richard

About reducing EXISTS sublink

[Richard Guo]

Hi hackers,

For EXISTS SubLink, in some cases the subquery can be reduced to
constant TRUE or FALSE, based on the knowledge that it's being used in
EXISTS(). One such case is when the subquery has aggregates without
GROUP BY or HAVING, and we know its result is exactly one row, unless
that row is discarded by LIMIT/OFFSET. (Greenplum does this.)

For example:

# explain (costs off) select * from a where exists
(select avg(i) from b where a.i = b.i);
QUERY PLAN
---
 Seq Scan on a
   Filter: (SubPlan 1)
   SubPlan 1
 ->  Aggregate
   ->  Seq Scan on b
 Filter: (a.i = i)
(6 rows)

This query can be reduced to:

# explain (costs off) select * from a where exists
(select avg(i) from b where a.i = b.i);
  QUERY PLAN
---
 Seq Scan on a
(1 row)

And likewise, for this query below:

# explain (costs off) select * from a where exists
(select avg(i) from b where a.i = b.i offset 1);
   QUERY PLAN
-
 Seq Scan on a
   Filter: (SubPlan 1)
   SubPlan 1
 ->  Limit
   ->  Aggregate
 ->  Seq Scan on b
   Filter: (a.i = i)
(7 rows)

It can be reduced to:

# explain (costs off) select * from a where exists
(select avg(i) from b where a.i = b.i offset 1);
QUERY PLAN
--
 Result
   One-Time Filter: false
(2 rows)

Is it worthwhile to add some codes for such optimization? If so, I can
try to propose a patch.

Thanks
Richard