Re: Update minimum SSL version

[Peter Eisentraut]

On 2019-12-02 17:39, Tom Lane wrote:

Robert Haas  writes:

... However, it would be worth putting in some
effort to make sure that we give a good error message if this happens.


That's an excellent point, but it looks like we're pretty good
already.  I tried the patch with openssl 0.9.8x, and got this
failure at server start:

FATAL:  ssl_min_protocol_version setting TLSv1.2 not supported by this build


That's the easy part, since it's under our control.  The other situation 
is if you connect with an old library to a newer server that has the 
raised ssl_min_protocol_version setting.  Then you get something like this:


psql: SSL error: tlsv1 alert protocol version

and on the server:

LOG:  could not accept SSL connection: unsupported protocol

Not great, but usable.

(What actually happens due to the default of PGSSLMODE=prefer is that 
psql/libpq will have the SSL connection attempt rejected and will 
connect using a non-SSL connection.)


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

[PATCH] Fix PostgreSQL 12.1 server build and install problems under MSYS2

[Guram Duka]

Hi hackers.

I made a patch fixing build and install problems under MSYS2, including
llvmjit.

I have tested this in my environment and it works, of course need more
extensive testing.
Attached is a patch that fixes it. Tag REL_12_1.

-- 
Best regards.
Guram Duka.


postgresql-12.1-msys2-v1.patch
Description: Binary data

Re: Increase footprint of %m and reduce strerror()

[Kyotaro Horiguchi]

At Fri, 29 Nov 2019 15:51:15 +0900, Michael Paquier  wrote 
in 
> Hi all,
> 
> Since commit d6c55de1, we support %m in the in-core port for printf
> and such.  And it seems to me that we could do better for the frontend
> code by reducing the dependency to strerror().
> 
> One advantage of doing a switch, or at least reduce the use of
> strerror(), would be to ease the work of translators with more error
> messages unified between the frontend and the backend.  A possible
> drawback is that this could be a cause of minor conflicts when
> back-patching.  Always easy enough to fix, still that can be 
> annoying.
> 
> Thoughts?

It sounds good to me.  Message unification (including printf) needs
somehow treating trailing new lines, though.  About translation
burden, I'm not sure how the message unification eases translators'
work. Identical messages of different commands appear having different
neighbours in different po files.

By the way aren't we going to have ereport on frontend?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Proposal: Add more compile-time asserts to expose inconsistencies.

[Michael Paquier]

On Mon, Dec 02, 2019 at 07:55:45AM -0800, Andres Freund wrote:
> On 2019-11-29 11:11:25 +0900, Michael Paquier wrote:
>> diff --git a/src/include/c.h b/src/include/c.h
>> index 00e41ac546..91d6d50e76 100644
>> --- a/src/include/c.h
>> +++ b/src/include/c.h
>> [...]
> 
> I think this a) needs an updated comment above, explaining this approach
> (note the explanation for the array approach) b) I still think we ought
> to work towards also using this implementation for StaticAssertStmt.

Sure.  I was not completely sure which addition would be helpful
except than adding in the main comment lock that Decl() is useful at
file scope.

> Now that I'm back from vacation, I'll try to take a stab at b). It
> should definitely doable to use the same approach for StaticAssertStmt,
> the problematic case might be StaticAssertExpr.

So you basically want to minimize the amount of code relying on
external compiler expressions?  Sounds like a plan.  At quick glance,
it seems that this should work.  I haven't tested though.  I'll wait
for what you come up with then.

>>  #else   /* C++ */
>>  #if defined(__cpp_static_assert) && __cpp_static_assert >= 200410
>> @@ -857,12 +862,16 @@ extern void ExceptionalCondition(const char 
>> *conditionName,
>>  static_assert(condition, errmessage)
>>  #define StaticAssertExpr(condition, errmessage) \
>>  ({ static_assert(condition, errmessage); })
>>
>> [...]
>>
>> +#define StaticAssertDecl(condition, errmessage) \
>> +extern void static_assert_func(int static_assert_failure[(condition) ? 
>> 1 : -1])
>> +#endif  /* 
>> __cpp_static_assert */
>>  #endif  /* C++ */
> 
> I wonder if it's worth moving the fallback implementation into an else
> branch that's "unified" between C and C++.

I suspect that you would run into problems with StaticAssertExpr() and
StaticAssertStmt().

>> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1),
>> + "LockTagTypeNames array inconsistency");
>> +
> 
> These error messages strike me as somewhat unhelpful. I'd probably just
> reword them as "array length mismatch" or something like that.

That's indeed better.  Now I think that it is useful to have the
structure name in the error message as well, no?

> I think this patch ought to include at least one StaticAssertDecl in a
> header, to make sure we get that part right across compilers. E.g. the
> one in PageIsVerified()?

No objections to have one, but I don't think that your suggestion is a
good choice.  This static assertion is based on size_t and BLCKSZ, and
is located close to a code path where we have a specific logic based
on both things.  If in the future this code gets removed, then we'd
likely miss to remove the static assertion if they are separated
across multiple files.
--
Michael


signature.asc
Description: PGP signature

Re: pgbench -i progress output on terminal

[Amit Langote]

On Wed, Dec 4, 2019 at 11:35 AM Michael Paquier  wrote:
>
> On Tue, Dec 03, 2019 at 10:30:35AM +0900, Amit Langote wrote:
> > How about adding a function, say print_progress_to_stderr(const char
> > *fmt,...), exposed to the front-end utilities and use it from
> > everywhere? Needless to say that it will contain the check for whether
> > stderr points to terminal or a file and print accordingly.
>
> I have considered this point, but that does not seem worth the
> complication as each tool has its own idea of the log output, its own
> idea of the log output timing and its own idea of when it is necessary
> to print the last newline when finishing to the output with '\r'.

Okay, seems more trouble than worth to design around all that.

> > Considering Fabien's comment on this, we will have to check which
> > other instances are printing information that is not very useful to
> > look at line-by-line.
>
> Thanks, applied the part for the initialization to HEAD.  I got to
> think about Fabien's point and it is true that for pgbench's
> --progress not keeping things on the same line for a terminal has
> advantages because the data printed is not cumulative: that's a
> summary of the previous state printed which can be compared.
>
> Note: the patch works on Windows, no problem.

Thanks for checking and committing the patch.

Regards,
Amit

Re: Using XLogFileNameP in critical section

[Michael Paquier]

On Tue, Dec 03, 2019 at 09:35:00AM -0300, Alvaro Herrera wrote:
> You didn't attach anything, but I concur about the low probability
> aspect: the assertion failure does not occur in production builds
> (obviously); and only an out-of-memory situation is a real problem
> when
> an fsync fails.  Anyway this should be a very localized fix, right?

Sorry.  You get something like the attached.  The recent refactoring
work you committed in this area causes already conflicts on
REL_12_STABLE.

> I'm not sure that the internationalization stuff in issue_xlog_fsync
> is correct.  I think the _() should be gettext_noop(), or alternatively
> the errmsg() should be errmsg_internal(); otherwise the translation is
> invoked twice.  (I didn't verify this.)

Hmm.  We actually do both in tablecmds.c:ATWrongRelkindError(), and
that's the code I was looking at yesterday when thinking about the
problem..  However, parse_agg.c, parse_expr.c and parse_func.c among
others like vacuumlazy.c use directly errmsg_internal() without
translating the string first.  So there is indeed duplicated work for
both.  Does the attached patch look correct to you?
--
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 6bc1a6b46d..01ca07b552 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -10171,7 +10171,7 @@ issue_xlog_fsync(int fd, XLogSegNo segno)
 		errno = save_errno;
 		ereport(PANIC,
 (errcode_for_file_access(),
- errmsg(msg, xlogfname)));
+ errmsg_internal(msg, xlogfname)));
 	}
 
 	pgstat_report_wait_end();
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5440eb9015..3d98af47b1 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -5230,7 +5230,7 @@ ATWrongRelkindError(Relation rel, int allowed_targets)
 
 	ereport(ERROR,
 			(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-			 errmsg(msg, RelationGetRelationName(rel;
+			 errmsg_internal(msg, RelationGetRelationName(rel;
 }
 
 /*
From 491faa2e92b5f637bb8b76eb1a9c2e93a6eec3a6 Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Wed, 4 Dec 2019 13:57:18 +0900
Subject: [PATCH] Remove use of XLogFileNameP() in error string generation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

XLogFileNameP() is a wrapper routine able to build a palloc'd string for
a WAL segment name, which is used in error strings.  There were several
code paths where it is called in a critical section, where memory
allocation is not allowed, triggering an assertion failure on PANIC
instead of generating a proper message.  Note that this could be a
problem on OOM when calling the routine, as any failure would be
escalated to a PANIC as well.  This removes all the callers of this
routine, to fix existing mistakes and prevent future ones, replacing the
incorrect locations with a logic using a fixed-size buffer.

Author: Masahiko Sawada
Reviewed-by: Michael Paquier, Álvaro Herrera
Discussion: https://postgr.es/m/ca+fd4k5gc9h4uowmlg9k_qfnrnkkdew+-afveob9yx7z8jn...@mail.gmail.com
---
 src/backend/access/transam/xlog.c | 78 ---
 src/backend/replication/walreceiver.c | 30 ---
 src/backend/replication/walsender.c   | 39 +++---
 3 files changed, 112 insertions(+), 35 deletions(-)

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 1a87042223..58c4ed9083 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2499,14 +2499,21 @@ XLogWrite(XLogwrtRqst WriteRqst, bool flexible)
 pgstat_report_wait_end();
 if (written <= 0)
 {
+	char		xlogfname[MAXFNAMELEN];
+	int			save_errno;
+
 	if (errno == EINTR)
 		continue;
+
+	save_errno = errno;
+	XLogFileName(xlogfname, ThisTimeLineID, openLogSegNo,
+ wal_segment_size);
+	errno = save_errno;
 	ereport(PANIC,
 			(errcode_for_file_access(),
 			 errmsg("could not write to log file %s "
 	"at offset %u, length %zu: %m",
-	XLogFileNameP(ThisTimeLineID, openLogSegNo),
-	startoffset, nleft)));
+	xlogfname, startoffset, nleft)));
 }
 nleft -= written;
 from += written;
@@ -3792,10 +3799,17 @@ XLogFileClose(void)
 #endif
 
 	if (close(openLogFile))
+	{
+		char		xlogfname[MAXFNAMELEN];
+		int			save_errno = errno;
+
+		XLogFileName(xlogfname, ThisTimeLineID, openLogSegNo, wal_segment_size);
+		errno = save_errno;
 		ereport(PANIC,
 (errcode_for_file_access(),
- errmsg("could not close file \"%s\": %m",
-		XLogFileNameP(ThisTimeLineID, openLogSegNo;
+ errmsg("could not close file \"%s\": %m", xlogfname)));
+	}
+
 	openLogFile = -1;
 }
 
@@ -5527,10 +5541,17 @@ exitArchiveRecovery(TimeLineID endTLI, XLogRecPtr endOfLog)
 		fd = XLogFileInit(startLogSegNo, _existent, true);
 
 		if (close(fd))
+		{
+			char		xlogfname[MAXFNAMELEN];
+			int			save_errno 

Re: Session WAL activity

[Kyotaro Horiguchi]

Hello.

At Tue, 3 Dec 2019 18:01:28 +0300, Konstantin Knizhnik 
 wrote in 
> Hi hackers,
> 
> One of our customers complains about that some sessions generates "too
> much WAL records".
> Certainly WAL activity doesn't indicate a problem itself: huge
> workload cause huge WAL activity.
> But them are trying to understand which clients produces so much
> database changes and complain that there is
> no way to get such information in Postgres. For example in Oracle this
> problems can be solved in this way:
> 
> http://www.dba-oracle.com/t_find_session_generating_high_redo.htm
> 
> Unfortunately there is actually no simple and accurate way to
> calculate amount of WAL produced by the particular session.
> It is possible to parse WAL (for example using pg_waldump), then using
> XID->pid mapping accumulate size of transactions produced by each
> backend.
> But this is very inconvenient and not DBA friendly approach.
> 
> I have implemented small patch which collects such statistic.
> I have added walWritten  field to PGPROC and increment it in
> CopyXLogRecordToWAL.
> It is possible to inspect this field using
> pg_stat_get_wal_activity(pid) function and also I have added
> pg_stat_wal_activity which just adds  wal_written to standard
> pg_activity view:
> 
> postgres=# select pid, backend_type, wal_written from
> pg_stat_wal_activity ;
>  pid  | backend_type | wal_written
> --+--+-
>  4405 | autovacuum launcher  |   0
>  4407 | logical replication launcher |   0
>  4750 | client backend   |   86195
>  4403 | background writer| 204
>  4402 | checkpointer | 328
>  4404 | walwriter|   0
> (6 rows)
> 
> 
> 
> I wonder if such extra statistic about session WAL activity is
> considered to be useful?
>
> The only problem with this approach from my point of view is adding 8
> bytes to PGPROC.
> But there are already so many fields in this structure
> (sizeof(PGPROC)=816), that adding yet another 8 bytes should not be
> noticeable.
> 
> Comments are welcome.

It seems to be useful to me. We also might want statistics of other
session IOs.  In that case the table name would be
"pg_stat_session/process_activity". We are aleady collecting most
kinds of the IO activity but it loses session information...

Briefly looking the patch, I have some comments on it.

As mentioned above, if we are intending future exantion of the
session-stats table, the name should be changed.

Backend status is more appropriate than PGPROC. See pgstat.c.

Some kind of locking is needed to update the fields on shared segment.
(LWLocks for PGPROC and PGSTAT_BEGIN/END_WRITE_ACTIVITY for
PgBackendStatus)

Knitpickings:

The patch contains a trace of older trial in
pg_stat_get_activity. Proc OID should be >= 8000 in
patches. src/include/catalog/unused_oids offers some OID for you.


regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: Protocol problem with GSSAPI encryption?

[Stephen Frost]

Greetings,

* Andrew Gierth (and...@tao11.riddles.org.uk) wrote:
> > "Peter" == Peter Eisentraut  writes:
> 
>  >> It seems to me that this is a bug in ProcessStartupPacket, which
>  >> should accept both GSS or SSL negotiation requests on a connection
>  >> (in either order). Maybe secure_done should be two flags rather than
>  >> one?
> 
>  Peter> I have also seen reports of that. I think your analysis is
>  Peter> correct.
> 
> I figure something along these lines for the fix. Anyone in a position
> to test this?

At least at first blush, I tend to agree with your analysis and patch.

I'll see about getting this actually set up and tested in the next week
or so (and maybe there's some way to also manage to have a regression
test for it..).

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Windows buildfarm members vs. new async-notify isolation test

[Tom Lane]

Amit Kapila  writes:
> On Tue, Dec 3, 2019 at 10:10 PM Tom Lane  wrote:
>> Hmm ... just looking at the code again, could it be that there's
>> no well-placed CHECK_FOR_INTERRUPTS?  Andrew, could you see if
>> injecting one in what 790026972 added to postgres.c helps?

> I also tried to analyze this failure and it seems this is a good bet,
> but I am also wondering why we have never seen such a timing issue in
> other somewhat similar tests.  For ex.,  one with comment (#
> Cross-backend notification delivery.).  Do they have a better way of
> ensuring that the notification will be received or is it purely
> coincidental that they haven't seen such a symptom?

TBH, my bet is that this *won't* fix it, but it seemed like an easy
thing to test.  For this to fix it, you'd have to suppose that we
never do a CHECK_FOR_INTERRUPTS during a COMMIT command, which is
improbable at best.

regards, tom lane

Re: Windows buildfarm members vs. new async-notify isolation test

[Amit Kapila]

On Tue, Dec 3, 2019 at 10:10 PM Tom Lane  wrote:
>
> In principle, the issue should not be there, because commits
> 790026972 et al should have ensured that the NOTIFY protocol
> message comes out before ReadyForQuery (and thus, libpq will
> absorb it before PQgetResult will return NULL).  I think the
> timing problem --- if that's what it is --- must be on the
> backend side; somehow the backend is not processing the
> inbound notify queue before it goes idle.
>
> Hmm ... just looking at the code again, could it be that there's
> no well-placed CHECK_FOR_INTERRUPTS?  Andrew, could you see if
> injecting one in what 790026972 added to postgres.c helps?
> That is,
>
> /*
>  * Also process incoming notifies, if any.  This is mostly to
>  * ensure stable behavior in tests: if any notifies were
>  * received during the just-finished transaction, they'll be
>  * seen by the client before ReadyForQuery is.
>  */
> +   CHECK_FOR_INTERRUPTS();
> if (notifyInterruptPending)
> ProcessNotifyInterrupt();
>

I also tried to analyze this failure and it seems this is a good bet,
but I am also wondering why we have never seen such a timing issue in
other somewhat similar tests.  For ex.,  one with comment (#
Cross-backend notification delivery.).  Do they have a better way of
ensuring that the notification will be received or is it purely
coincidental that they haven't seen such a symptom?

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

Re: Using XLogFileNameP in critical section

[Michael Paquier]

On Tue, Dec 03, 2019 at 11:24:57AM -0500, Tom Lane wrote:
> So it seems that the problem might really be a faulty rule in our
> MSVC build script about when postgres.def needs to be regenerated?
> Or else it's some weird caching problem on drongo --- the lack of
> complaints from other Windows critters might point the finger
> that way.

Yes, I saw the failure from the buildfarm logs, but I got to the
conclusion that the animal just got crazy with a portion of its
caching because there are no more references to routine removed.  So I
did not bother much.

FWIW, I have seen sometimes similar warnings regarding conflicting
projects when doing a compilation on MSVC, applying a patch and then
recompiling (because that's just faster than recompiling the whole).
Perhaps we could do better here.  I am not completely sure what
though, it's not a problem I have spent much brain-time on.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Incomplete startup packet errors

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Jobin Augustine  writes:
> > However, Checking whether the port is open is resulting in error log like:
> > 2019-11-25 14:03:44.414 IST [14475] LOG:  invalid length of startup packet
> > Yes, This is different from "Incomplete startup packet" discussed here.
> 
> > Steps to reproduce:
> > $ telnet localhost 5432
> 
> Well, the agreed-to behavior change was to not log anything if the
> connection is closed without any data having been sent.  If the
> client *does* send something, and it doesn't look like a valid
> connection request, I think we absolutely should log that.

Agreed.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: [HACKERS] Block level parallel vacuum

[Dilip Kumar]

On Wed, Dec 4, 2019 at 9:12 AM Amit Kapila  wrote:
>
> On Wed, Dec 4, 2019 at 1:58 AM Masahiko Sawada
>  wrote:
> >
> > On Tue, 3 Dec 2019 at 11:55, Amit Kapila  wrote:
> > >
> > In your code, I think if two workers enter to compute_parallel_delay
> > function at the same time, they add their local balance to
> > VacuumSharedCostBalance and both workers sleep because both values
> > reach the VacuumCostLimit.
> >
>
> True, but isn't it more appropriate because the local cost of any
> worker should be ideally added to shared cost as soon as it occurred?
> I mean to say that we are not adding any cost in shared balance
> without actually incurring it.   Then we also consider the individual
> worker's local balance as well and sleep according to local balance.

Even I think it is better to add the balance to the shared balance at
the earliest opportunity.  Just consider the case that there are 5
workers and all have I/O balance of 20, and VacuumCostLimit is 50.  So
Actually, there combined balance is 100 (which is double of the
VacuumCostLimit) but if we don't add immediately then none of the
workers will sleep and it may go to the next cycle which is not very
good. OTOH, if we add 20 immediately then check the shared balance
then all the workers might go for sleep if their local balances have
reached the limit but they will only sleep in proportion to their
local balance.  So IMHO, adding the current balance to shared balance
early is more close to the model we are trying to implement i.e.
shared cost accounting.

>
> >
> > > 2. I think if we cam somehow disallow very small indexes to use parallel 
> > > workers, then it will be better.   Can we use  
> > > min_parallel_index_scan_size to decide whether a particular index can 
> > > participate in a parallel vacuum?
> >
> > I think it's a good idea but I'm concerned that the default value of
> > min_parallel_index_scan_size, 512kB, is too small for parallel vacuum
> > purpose. Given that people who want to use parallel vacuum are likely
> > to have a big table the indexes that can be skipped by the default
> > value would be only brin indexes, I think.
> >
>
> Yeah or probably hash indexes in some cases.
>
> > Also I guess that the
> > reason why the default value is small is that
> > min_parallel_index_scan_size compares to the number of blocks being
> > scanned during index scan, not whole index. On the other hand in
> > parallel vacuum we will compare it to the whole index blocks because
> > the index vacuuming is always full scan. So I'm also concerned that
> > user will get confused about reasonable setting.
> >
>
> This setting is about how much of index we are going to scan, so I am
> not sure if it matters whether it is part or full index scan.  Also,
> in an index scan, we will launch multiple workers to scan that index
> and here we will consider launching just one worker.
>
> > As another idea how about using min_parallel_table_scan_size instead?
> >
>
> Hmm, yeah, that can be another option, but it might not be a good idea
> for partial indexes.
>
> > That is, we cannot do parallel vacuum on the table smaller than that
> > value.
> >
>
> Yeah, that makes sense, but I feel if we can directly target index
> scan size that may be a better option.  If we can't use
> min_parallel_index_scan_size, then we can consider this.
>
> --
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com



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

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Wed, Dec 4, 2019 at 1:58 AM Masahiko Sawada
 wrote:
>
> On Tue, 3 Dec 2019 at 11:55, Amit Kapila  wrote:
> >
> In your code, I think if two workers enter to compute_parallel_delay
> function at the same time, they add their local balance to
> VacuumSharedCostBalance and both workers sleep because both values
> reach the VacuumCostLimit.
>

True, but isn't it more appropriate because the local cost of any
worker should be ideally added to shared cost as soon as it occurred?
I mean to say that we are not adding any cost in shared balance
without actually incurring it.   Then we also consider the individual
worker's local balance as well and sleep according to local balance.

>
> > 2. I think if we cam somehow disallow very small indexes to use parallel 
> > workers, then it will be better.   Can we use  min_parallel_index_scan_size 
> > to decide whether a particular index can participate in a parallel vacuum?
>
> I think it's a good idea but I'm concerned that the default value of
> min_parallel_index_scan_size, 512kB, is too small for parallel vacuum
> purpose. Given that people who want to use parallel vacuum are likely
> to have a big table the indexes that can be skipped by the default
> value would be only brin indexes, I think.
>

Yeah or probably hash indexes in some cases.

> Also I guess that the
> reason why the default value is small is that
> min_parallel_index_scan_size compares to the number of blocks being
> scanned during index scan, not whole index. On the other hand in
> parallel vacuum we will compare it to the whole index blocks because
> the index vacuuming is always full scan. So I'm also concerned that
> user will get confused about reasonable setting.
>

This setting is about how much of index we are going to scan, so I am
not sure if it matters whether it is part or full index scan.  Also,
in an index scan, we will launch multiple workers to scan that index
and here we will consider launching just one worker.

> As another idea how about using min_parallel_table_scan_size instead?
>

Hmm, yeah, that can be another option, but it might not be a good idea
for partial indexes.

> That is, we cannot do parallel vacuum on the table smaller than that
> value.
>

Yeah, that makes sense, but I feel if we can directly target index
scan size that may be a better option.  If we can't use
min_parallel_index_scan_size, then we can consider this.

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

Re: pg_upgrade fails with non-standard ACL

[Arthur Zakirov]

On 2019/12/01 23:58, Grigory Smolkin wrote:

On 11/29/19 11:07 AM, Artur Zakirov wrote:

New version of the patch differs from the previous:
- it doesn't generate script to revoke conflicting permissions (but 
the patch can be fixed easily)
- generates file incompatible_objects_for_acl.txt to report which 
objects changed their signatures
- uses pg_shdepend and pg_depend catalogs to get objects with custom 
privileges

- uses pg_describe_object() to find objects with different signatures

Currently relations, attributes, languages, functions and procedures 
are scanned. According to the documentation foreign databases, 
foreign-data wrappers, foreign servers, schemas and tablespaces also 
support ACLs. But some of them doesn't have entries initialized during 
initdb, others like schemas and tablespaces didn't change their names. 
So the patch doesn't scan such objects.


Grigory it would be great if you'll try the patch. I tested it but I 
could miss something.


I`ve tested the patch on upgrade from 9.5 to master and it works now, 
thank you.


Great!

But I think that 'incompatible_objects_for_acl.txt' is not a very 
informative way of reporting to user the source of the problem.
There is no mentions of rolenames that holds permissions, that prevents 
the upgrade, and even if they were, it is still up to user to conjure an 
script to revoke all those grants, which is not a very user-friendly.


I tried to find some pattern how pg_upgrade decides to log list of 
problem objects or to generate SQL script file to execute by user. 
Nowadays only "Checking for large objects" and "Checking for hash 
indexes" generate SQL script files and log WARNING message.


I think it should generate 'catalog_procedures.sql' script as in 
previous version with all REVOKE statements, that are required to 
execute for pg_upgrade to work.


I updated the patch. It generates "revoke_objects.sql" (similar to v3 
patch) now and doesn't rely on --check option. It also logs still FATAL 
message because it seems pg_upgrade should stop here since it fails 
later if there are objects with changed identities.


The patch doesn't generate "incompatible_objects_for_acl.txt" now 
because it would duplicate "revoke_objects.sql".


It now uses pg_identify_object() instead of pg_describe_object(), it is 
easier to get column names with it.


--
Arthur
diff --git a/src/bin/pg_upgrade/check.c b/src/bin/pg_upgrade/check.c
index cdd8788b9e..20a3f26289 100644
--- a/src/bin/pg_upgrade/check.c
+++ b/src/bin/pg_upgrade/check.c
@@ -16,6 +16,7 @@

 static void check_new_cluster_is_empty(void);
 static void check_databases_are_compatible(void);
+static void check_for_changed_signatures(void);
 static void check_locale_and_encoding(DbInfo *olddb, DbInfo *newdb);
 static bool equivalent_locale(int category, const char *loca, const char 
*locb);
 static void check_is_install_user(ClusterInfo *cluster);
@@ -142,6 +143,8 @@ check_and_dump_old_cluster(bool live_check)
if (GET_MAJOR_VERSION(old_cluster.major_version) <= 804)
new_9_0_populate_pg_largeobject_metadata(_cluster, true);

+   get_non_default_acl_infos(_cluster);
+
/*
 * While not a check option, we do this now because this is the only 
time
 * the old server is running.
@@ -161,6 +164,7 @@ check_new_cluster(void)

check_new_cluster_is_empty();
check_databases_are_compatible();
+   check_for_changed_signatures();

check_loadable_libraries();

@@ -443,6 +447,196 @@ check_databases_are_compatible(void)
}
 }

+/*
+ * Find the location of the last dot, return NULL if not found.
+ */
+static char *
+last_dot_location(const char *identity)
+{
+   const char *p,
+  *ret = NULL;
+
+   for (p = identity; *p; p++)
+   if (*p == '.')
+   ret = p;
+   return unconstify(char *, ret);
+}
+
+/*
+ * check_for_changed_signatures()
+ *
+ * Checks that the old cluster doesn't have non-default ACL's for system 
objects
+ * which had different signatures in the new cluster.  Otherwise generates
+ * revoke_objects.sql.
+ */
+static void
+check_for_changed_signatures(void)
+{
+   PGconn *conn;
+   PGresult   *res;
+   int ntups;
+   int i_obj_ident;
+   int dbnum;
+   boolneed_check = false;
+   FILE   *script = NULL;
+   boolfound_changed = false;
+   charoutput_path[MAXPGPATH];
+
+   prep_status("Checking for system objects to grant or revoke 
privileges");
+
+   for (dbnum = 0; dbnum < old_cluster.dbarr.ndbs; dbnum++)
+   if (old_cluster.dbarr.dbs[dbnum].non_def_acl_arr.nacls > 0)
+   {
+   need_check = true;
+   break;
+   }
+   /*
+* The old cluster doesn't have system objects with non-default ACL so
+* quickly exit.
+ 

Re: pgbench -i progress output on terminal

[Michael Paquier]

On Tue, Dec 03, 2019 at 10:30:35AM +0900, Amit Langote wrote:
> How about adding a function, say print_progress_to_stderr(const char
> *fmt,...), exposed to the front-end utilities and use it from
> everywhere? Needless to say that it will contain the check for whether
> stderr points to terminal or a file and print accordingly.

I have considered this point, but that does not seem worth the
complication as each tool has its own idea of the log output, its own
idea of the log output timing and its own idea of when it is necessary
to print the last newline when finishing to the output with '\r'.

> Considering Fabien's comment on this, we will have to check which
> other instances are printing information that is not very useful to
> look at line-by-line.

Thanks, applied the part for the initialization to HEAD.  I got to
think about Fabien's point and it is true that for pgbench's
--progress not keeping things on the same line for a terminal has
advantages because the data printed is not cumulative: that's a
summary of the previous state printed which can be compared.

Note: the patch works on Windows, no problem.
--
Michael


signature.asc
Description: PGP signature

[Proposal] Level4 Warnings show many shadow vars

[Ranier Vilela]

Hi,
I believe PostgreSQL can benefit from changing the alert level of compilation 
warnings.
The current Level3 level for windows does not show any alerts, but that does 
not mean that there are no problems.
Changing the level to Level4 and its equivalent for GCC in Unix environments 
will show many warnings for shadow variables, including global variables.
True, there will also be many silly alerts that can be safely disabled.
Shadow variables, although they may not currently represent bugs, may be hiding 
errors, or at the very least, it is a waste of variable declaration.
With the current Level3 level, development is no longer checking and correcting 
shadow variables.

Any comments?

Best regards,
Ranier Vilela

Re: fe-utils - share query cancellation code

[Michael Paquier]

On Tue, Dec 03, 2019 at 01:11:27PM +0100, Fabien COELHO wrote:
> Looks fine to me: patch applies, compiles, runs.

Thanks for double-checking.  Done.
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Addition of JetBrains project directory to .gitignore

[Michael Paquier]

On Tue, Dec 03, 2019 at 10:07:08AM -0500, Tom Lane wrote:
> As a point of reference, I have
> 
> $ cat ~/.gitexclude 
> *~
> *.orig
> 
> to suppress emacs backup files and patch backup files respectively.
> Somebody who prefers another editor would have no use for *~.

Here are extra entries I use for example:
# Files created by vim for unsaved changes
.*.swp
# Files created by emacs for unsaved changes
.#*
# Temporary files created during compilation
*.o-*
# Tags generated by etags or ctags
TAGS
tags
# Files created by ./configure
conftest.c
conftest.err
confdefs.h
--
Michael


signature.asc
Description: PGP signature

Re: Protocol problem with GSSAPI encryption?

[Andrew Gierth]

> "Peter" == Peter Eisentraut  writes:

 >> It seems to me that this is a bug in ProcessStartupPacket, which
 >> should accept both GSS or SSL negotiation requests on a connection
 >> (in either order). Maybe secure_done should be two flags rather than
 >> one?

 Peter> I have also seen reports of that. I think your analysis is
 Peter> correct.

I figure something along these lines for the fix. Anyone in a position
to test this?

-- 
Andrew (irc:RhodiumToad)

diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 9ff2832c00..1b51d4916d 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -404,7 +404,7 @@ static void BackendRun(Port *port) pg_attribute_noreturn();
 static void ExitPostmaster(int status) pg_attribute_noreturn();
 static int	ServerLoop(void);
 static int	BackendStartup(Port *port);
-static int	ProcessStartupPacket(Port *port, bool secure_done);
+static int	ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done);
 static void SendNegotiateProtocolVersion(List *unrecognized_protocol_options);
 static void processCancelRequest(Port *port, void *pkt);
 static int	initMasks(fd_set *rmask);
@@ -1914,11 +1914,15 @@ initMasks(fd_set *rmask)
  * send anything to the client, which would typically be appropriate
  * if we detect a communications failure.)
  *
- * Set secure_done when negotiation of an encrypted layer (currently, TLS or
- * GSSAPI) is already completed.
+ * Set ssl_done and/or gss_done when negotiation of an encrypted layer
+ * (currently, TLS or GSSAPI) is completed. A successful negotiation of either
+ * encryption layer sets both flags, but a rejected negotiation sets only the
+ * flag for that layer, since the client may wish to try the other one. We
+ * should make no assumption here about the order in which the client may make
+ * requests.
  */
 static int
-ProcessStartupPacket(Port *port, bool secure_done)
+ProcessStartupPacket(Port *port, bool ssl_done, bool gss_done)
 {
 	int32		len;
 	void	   *buf;
@@ -1951,7 +1955,7 @@ ProcessStartupPacket(Port *port, bool secure_done)
 	if (pq_getbytes(((char *) ) + 1, 3) == EOF)
 	{
 		/* Got a partial length word, so bleat about that */
-		if (!secure_done)
+		if (!ssl_done && !gss_done)
 			ereport(COMMERROR,
 	(errcode(ERRCODE_PROTOCOL_VIOLATION),
 	 errmsg("incomplete startup packet")));
@@ -2003,7 +2007,7 @@ ProcessStartupPacket(Port *port, bool secure_done)
 		return STATUS_ERROR;
 	}
 
-	if (proto == NEGOTIATE_SSL_CODE && !secure_done)
+	if (proto == NEGOTIATE_SSL_CODE && !ssl_done)
 	{
 		char		SSLok;
 
@@ -2032,11 +2036,14 @@ retry1:
 		if (SSLok == 'S' && secure_open_server(port) == -1)
 			return STATUS_ERROR;
 #endif
-		/* regular startup packet, cancel, etc packet should follow... */
-		/* but not another SSL negotiation request */
-		return ProcessStartupPacket(port, true);
+		/*
+		 * regular startup packet, cancel, etc packet should follow, but not
+		 * another SSL negotiation request, and a GSS request should only
+		 * follow if SSL was rejected (client may negotiate in either order)
+		 */
+		return ProcessStartupPacket(port, true, SSLok == 'S');
 	}
-	else if (proto == NEGOTIATE_GSS_CODE && !secure_done)
+	else if (proto == NEGOTIATE_GSS_CODE && !gss_done)
 	{
 		char		GSSok = 'N';
 #ifdef ENABLE_GSS
@@ -2059,8 +2066,12 @@ retry1:
 		if (GSSok == 'G' && secure_open_gssapi(port) == -1)
 			return STATUS_ERROR;
 #endif
-		/* Won't ever see more than one negotiation request */
-		return ProcessStartupPacket(port, true);
+		/*
+		 * regular startup packet, cancel, etc packet should follow, but not
+		 * another GSS negotiation request, and an SSL request should only
+		 * follow if GSS was rejected (client may negotiate in either order)
+		 */
+		return ProcessStartupPacket(port, GSSok == 'G', true);
 	}
 
 	/* Could add additional special packet types here */
@@ -4400,7 +4411,7 @@ BackendInitialize(Port *port)
 	 * Receive the startup packet (which might turn out to be a cancel request
 	 * packet).
 	 */
-	status = ProcessStartupPacket(port, false);
+	status = ProcessStartupPacket(port, false, false);
 
 	/*
 	 * Stop here if it was bad or a cancel packet.  ProcessStartupPacket

Re: Runtime pruning problem

[Tom Lane]

Alvaro Herrera  writes:
> On 2019-Jul-30, Tom Lane wrote:
>> The portion of this below the Append is fine, but I argue that
>> the Vars above the Append should say "part", not "part_p1".
>> In that way they'd look the same regardless of which partitions
>> have been pruned or not.

> So is anyone working on a patch to use this approach?

I spent some more time on this today, and successfully converted
ruleutils.c back to dealing only in Plan trees not PlanState trees.
The hard part of this turned out to be that in a Plan tree, it's
not so easy to identify subplans and initplans; the links that
simplify that in the existing ruleutils code get set up while
initializing the PlanState tree.  I had to do two things to make
it work:

* To cope with CTEScans and initPlans, ruleutils now needs access to the
PlannedStmt->subplans list, which can be set up along with the rtable.
I thought adding that as a separate argument wasn't very forward-looking,
so instead I changed the API of that function to pass the PlannedStmt.

* To cope with SubPlans, I changed the definition of the "ancestors"
list so that it includes SubPlans along with regular Plan nodes.
This is slightly squirrely, because SubPlan isn't a subclass of Plan,
but it seems to work well.  Notably, we don't have to search for
relevant SubPlan nodes in find_param_referent().  We'll just arrive
at them naturally while chasing up the ancestors list.

I don't think this is committable as it stands, because there are
a couple of undesirable changes in partition_prune.out.  Those test
cases are explaining queries in which the first child of a MergeAppend
gets pruned during executor start.  That results in ExplainPreScanNode
not seeing that node, so it deems the associated RTE to be unreferenced,
so select_rtable_names_for_explain doesn't assign that RTE an alias.
But then when we drill down for a referent for a Var above the
MergeAppend, we go to the first child of the MergeAppend (not the
MergeAppendState), ie exactly the RTE that was deemed unreferenced.
So we end up with no table alias to print.

That's not ruleutils.c's fault obviously: it did what it was told.
And it ties right into the question that's at the heart of this
discussion, ie what do we want to print for such Vars?  So I think
this patch is all right as a component of the full fix, but now we
have to move on to the main event.  I have some ideas about what
to do next, but they're not fully baked yet.

regards, tom lane

diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 62fb343..4d9d668 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -689,8 +689,8 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc)
 	es->rtable = queryDesc->plannedstmt->rtable;
 	ExplainPreScanNode(queryDesc->planstate, _used);
 	es->rtable_names = select_rtable_names_for_explain(es->rtable, rels_used);
-	es->deparse_cxt = deparse_context_for_plan_rtable(es->rtable,
-	  es->rtable_names);
+	es->deparse_cxt = deparse_context_for_plan_tree(queryDesc->plannedstmt,
+	es->rtable_names);
 	es->printed_subplans = NULL;
 
 	/*
@@ -1049,8 +1049,8 @@ ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used)
  * We need to work from a PlanState node, not just a Plan node, in order to
  * get at the instrumentation data (if any) as well as the list of subplans.
  *
- * ancestors is a list of parent PlanState nodes, most-closely-nested first.
- * These are needed in order to interpret PARAM_EXEC Params.
+ * ancestors is a list of parent Plan and SubPlan nodes, most-closely-nested
+ * first.  These are needed in order to interpret PARAM_EXEC Params.
  *
  * relationship describes the relationship of this plan node to its parent
  * (eg, "Outer", "Inner"); it can be null at top level.  plan_name is an
@@ -1953,8 +1953,8 @@ ExplainNode(PlanState *planstate, List *ancestors,
 	if (haschildren)
 	{
 		ExplainOpenGroup("Plans", "Plans", false, es);
-		/* Pass current PlanState as head of ancestors list for children */
-		ancestors = lcons(planstate, ancestors);
+		/* Pass current Plan as head of ancestors list for children */
+		ancestors = lcons(plan, ancestors);
 	}
 
 	/* initPlan-s */
@@ -2075,9 +2075,9 @@ show_plan_tlist(PlanState *planstate, List *ancestors, ExplainState *es)
 		return;
 
 	/* Set up deparsing context */
-	context = set_deparse_context_planstate(es->deparse_cxt,
-			(Node *) planstate,
-			ancestors);
+	context = set_deparse_context_plan(es->deparse_cxt,
+	   plan,
+	   ancestors);
 	useprefix = list_length(es->rtable) > 1;
 
 	/* Deparse each result column (we now include resjunk ones) */
@@ -2106,9 +2106,9 @@ show_expression(Node *node, const char *qlabel,
 	char	   *exprstr;
 
 	/* Set up deparsing context */
-	context = set_deparse_context_planstate(es->deparse_cxt,
-			(Node *) planstate,
-			ancestors);
+	context = 

Re: Online checksums patch - once again

[Daniel Gustafsson]

> On 1 Dec 2019, at 03:32, Michael Paquier  wrote:

> The latest patch does not apply, could you send a rebase?  Moved it to
> next CF, waiting on author.

Attached is a rebased v14 patchset on top of maser.  The Global Barriers patch
is left as a prerequisite, but it will obviously be dropped, or be
significantly changed, once the work Robert is doing with ProcSignalBarrier
lands.

cheers ./daniel



0001-Global-Barriers.patch
Description: Binary data


0002-Online-Checksums-v14.patch
Description: Binary data

Re: log bind parameter values on error

[Alvaro Herrera]

On 2019-Dec-03, Alvaro Herrera wrote:

> Now, I have to say that this doesn't make me terribly happy, because I
> would like the additional ability to limit the printed values to N
> bytes.  This means the new function would have to have an additional
> argument to indicate the maximum length (pass 0 to print all args
> whole) ... and the logic then because more involved because we need
> logic to stop printing early.

So after actually writing this, it seems that it's better to have a
separate path for this; otherwise you need to scan the whole input
string (strchr or friends), which might be long.  I tried to see how to
make strnlen() work for us, but I found no simple way to print the "..."
at the end.  So this just runs once per char.  I think that's okay,
since the intended usage is to print a few dozen bytes, like
ExecBuildSlotValueDescription does (64 bytes is what all its callers
do).

Another point: this implementation is not strict about how many
input characters it prints.  It counts up to maxlen *output* characters,
which means if it duplicates a ', that's one less input char printed.
(Given the usage of this function, this seems a feature rather than a
bug.  The first implementation did the opposite, and on the whole it
seemed worse.)

If anyone can suggest a smarter implementation, I'm all ears.

(Maybe do strnlen(maxlen), then count strnlen(1) starting at that point
-- so if that returns >=1, print the "..."?)

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

Re: Patch to document base64 encoding

[Karl O. Pinc]

Hi,

Attached is doc_base64_v11.patch

This addresses Tom's concerns.  Functions
that operate on both strings and bytea
(e.g. length(text) and length(bytea))
are documented separately, one with
string functions and one with binary
string functions.

In this iteration I have also:

Added a sub-section for the functions
which convert between text and bytea.

Added some index entries.

Provided a link in the hash functions to
the text about why md5() returns text
not bytea.

Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 57a1539506..aac9065e16 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -1432,6 +1432,13 @@

 

+ Functions which convert, both to and from, strings and
+ the bytea type
+ are documented
+ separately.
+   
+
+   
 SQL defines some string functions that use
 key words, rather than commas, to separate
 arguments.  Details are in
@@ -1793,101 +1800,6 @@
   
 
   
-   
-
- convert
-
-convert(string bytea,
-src_encoding name,
-dest_encoding name)
-   
-   bytea
-   
-Convert string to dest_encoding.  The
-original encoding is specified by
-src_encoding. The
-string must be valid in this encoding.
-Conversions can be defined by CREATE CONVERSION.
-Also there are some predefined conversions. See  for available conversions.
-   
-   convert('text_in_utf8', 'UTF8', 'LATIN1')
-   text_in_utf8 represented in Latin-1
-   encoding (ISO 8859-1)
-  
-
-  
-   
-
- convert_from
-
-convert_from(string bytea,
-src_encoding name)
-   
-   text
-   
-Convert string to the database encoding.  The original encoding
-is specified by src_encoding. The
-string must be valid in this encoding.
-   
-   convert_from('text_in_utf8', 'UTF8')
-   text_in_utf8 represented in the current database encoding
-  
-
-  
-   
-
- convert_to
-
-convert_to(string text,
-dest_encoding name)
-   
-   bytea
-   
-Convert string to dest_encoding.
-   
-   convert_to('some text', 'UTF8')
-   some text represented in the UTF8 encoding
-  
-
-  
-   
-
- decode
-
-decode(string text,
-format text)
-   
-   bytea
-   
-Decode binary data from textual representation in string.
-Options for format are same as in encode.
-   
-   decode('MTIzAAE=', 'base64')
-   \x3132330001
-  
-
-  
-   
-
- encode
-
-encode(data bytea,
-format text)
-   
-   text
-   
-Encode binary data into a textual representation.  Supported
-formats are: base64, hex, escape.
-escape converts zero bytes and high-bit-set bytes to
-octal sequences (\nnn) and
-doubles backslashes.
-   
-   encode('123\000\001', 'base64')
-   MTIzAAE=
-  
-
-  

 
  format
@@ -1955,19 +1867,6 @@
   
 
   
-   length(string bytea,
-encoding name )
-   int
-   
-Number of characters in string in the given
-encoding. The string
-must be valid in this encoding.
-   
-   length('jose', 'UTF8')
-   4
-  
-
-  

 
  lpad
@@ -2014,7 +1913,7 @@
 
 md5(string)

-   text
+   text*

 Calculates the MD5 hash of string,
 returning the result in hexadecimal
@@ -2333,6 +2232,66 @@
   

 
+ sha224
+
+sha224(string)
+   
+   bytea*
+   
+SHA-224 hash
+   
+   sha224('abc')
+   \x23097d223405d8228642a477bda255b32aadbce4bda0b3f7e36c9da7
+  
+
+  
+   
+
+ sha256
+
+sha256(string)
+   
+   bytea*
+   
+SHA-256 hash
+   
+   sha256('abc')
+   \xba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad
+  
+
+  
+   
+
+ sha384
+
+sha384(string)
+   
+   bytea*
+   
+SHA-384 hash
+   
+   sha384('abc')
+   \xcb00753f45a35e8bb5a03d699ac65007272c32ab0eded1631a8b605a43ff5bed8086072ba1e7cc2358baeca134c825a7
+  
+
+  
+   
+
+ sha512
+
+sha512(string)
+   
+   bytea*
+   
+SHA-512 hash
+   
+   sha512('abc')
+   \xddaf35a193617abacc417349ae20413112e6fa4e89a97ea20a964b55d39a2192992a274fc1a836ba3c23a3feebbd454d4423643ce80e2a9ac94fa54ca49f
+  
+
+  
+   
+
  split_part
 
 

Re: How to prohibit parallel scan through tableam?

[Andres Freund]

Hi,

On 2019-11-27 16:10:20 +0300, Konstantin Knizhnik wrote:
> On 27.11.2019 15:12, Rafia Sabih wrote:
> > On Wed, 27 Nov 2019 at 12:33, Konstantin Knizhnik
> > mailto:k.knizh...@postgrespro.ru>> wrote:
> > 
> > Hi hackers,
> > 
> > I wonder how it is possible to prohibit parallel scan for the
> > external
> > storage accessed through tableam?
> > For example if I want to implement specialized tableam for fast
> > access
> > to temp tables, how can I inform optimizer that
> > parallel scan is not possible (because table data is local to the
> > backend)?
> > 
> >  How about setting parallel_setup_cost to disable_cost in costsize.c for
> > your specific scan method.
> 
> How can I do it if i just implementing my AM and not going to change any
> postgres code?

I think a set_rel_pathlist hook that prevents parallel paths from being
considered would be your best bet for now. But I encourage you to
suggest a patch to tableam to support it properly in future releases.

Greetings,

Andres Freund

Re: How to prohibit parallel scan through tableam?

[Andres Freund]

Hi,

On 2019-11-27 14:33:42 +0300, Konstantin Knizhnik wrote:
> I wonder how it is possible to prohibit parallel scan for the external
> storage accessed through tableam?
> For example if I want to implement specialized tableam for fast access to
> temp tables, how can I inform optimizer that
> parallel scan is not possible (because table data is local to the backend)?

I don't think there currently is a good way to do so - but it shouldn't
be hard to add that capability.

Greetings,

Andres Freund

Re: Using XLogFileNameP in critical section

[Masahiko Sawada]

On Tue, 3 Dec 2019 at 07:09, Michael Paquier  wrote:
>
> On Mon, Dec 02, 2019 at 10:14:58PM +0100, Masahiko Sawada wrote:
> > Agreed. I've attached the updated version patch. Please review it.
>
> Thanks, applied on HEAD after a few edits.  gettext() does not set
> errno, so the new style of issue_xlog_fsync() is actually fine.

Thanks!

> Please note that there was one mistake in the patch: you forgot to
> assign back errno in assign_xlog_sync_method() after generating the
> file name.

My bad. Thank you for fixing it.

Regards,

-- 
Masahiko Sawadahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Block level parallel vacuum

[Masahiko Sawada]

On Tue, 3 Dec 2019 at 11:57, Amit Kapila  wrote:
>
> On Tue, Dec 3, 2019 at 4:25 PM Amit Kapila  wrote:
>>
>> Few other things, I would like you to consider.
>> 1.  I think disable_parallel_leader_participation related code can be 
>> extracted into a separate patch as it is mainly a debug/test aid.  You can 
>> also fix the problem reported by Mahendra in that context.
>>
>> 2. I think if we cam somehow disallow very small indexes to use parallel 
>> workers, then it will be better.   Can we use  min_parallel_index_scan_size 
>> to decide whether a particular index can participate in a parallel vacuum?
>
>
> Forgot one minor point.  Please run pgindent on all the patches.

Got it. I will run pgindent before sending patch from next time.

Regards,

-- 
Masahiko Sawadahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Allow relocatable extension to use @extschema@?

[Alexander Korotkov]

On Tue, Dec 3, 2019 at 6:18 PM Tom Lane  wrote:
> Alexander Korotkov  writes:
> > But nevertheless should we allow relocatable extension to use
> > @extschema@.  Any thoughts?
>
> No.  The reasoning in the comment still holds good: if you embed
> @extschema@ in an object's definition, it becomes nonrelocatable.

I see, allowing @extschema@ in non-relocatable extension provides easy
way to shoot yourself in the foot.

However, it might be still useful to be able to distinguish extension
and core object in upgrade script of relocatable extensions.  What
about (optional) way to set search_path to @extschema@, pg_catalog
instead of just @extschema@?

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Block level parallel vacuum

[Masahiko Sawada]

On Tue, 3 Dec 2019 at 11:55, Amit Kapila  wrote:
>
> On Tue, Dec 3, 2019 at 12:56 AM Masahiko Sawada 
>  wrote:
>>
>> On Sun, 1 Dec 2019 at 18:31, Sergei Kornilov  wrote:
>> >
>> > Hi
>> >
>> > > I think I got your point. Your proposal is that it's more efficient if
>> > > we make the leader process vacuum the index that can be processed only
>> > > the leader process (i.e. indexes not supporting parallel index vacuum)
>> > > while workers are processing indexes supporting parallel index vacuum,
>> > > right? That way, we can process indexes in parallel as much as
>> > > possible.
>> >
>> > Right
>> >
>> > > So maybe we can call vacuum_or_cleanup_skipped_indexes first
>> > > and then call vacuum_or_cleanup_indexes_worker. But I'm not sure that
>> > > there are parallel-safe remaining indexes after the leader finished
>> > > vacuum_or_cleanup_indexes_worker, as described on your proposal.
>> >
>> > I meant that after processing missing indexes (not supporting parallel 
>> > index vacuum), the leader can start processing indexes that support the 
>> > parallel index vacuum, along with parallel workers.
>> > Exactly call vacuum_or_cleanup_skipped_indexes after start parallel 
>> > workers but before vacuum_or_cleanup_indexes_worker or something with 
>> > similar effect.
>> > If we have 0 missed indexes - parallel vacuum will run as in current 
>> > implementation, with leader participation.
>>
>> I think your idea might not work well in some cases.
>
>
> Good point.  I am also not sure whether it is a good idea to make the 
> suggested change, but I think adding a comment on those lines is not a bad 
> idea which I have done in the attached patch.

Thank you for updating the patch!

>
> I have made some other changes as well.
> 1.
> + if (VacuumSharedCostBalance != NULL)
>   {
> - double msec;
> + int nworkers = pg_atomic_read_u32
> (VacuumActiveNWorkers);
> +
> + /* At least count itself */
> + Assert(nworkers >= 1);
> +
> + /* Update the shared cost
> balance value atomically */
> + while (true)
> + {
> + uint32 shared_balance;
> + uint32 new_balance;
> +
> uint32 local_balance;
> +
> + msec = 0;
> +
> + /* compute new balance by adding the local value */
> +
> shared_balance = pg_atomic_read_u32(VacuumSharedCostBalance);
> + new_balance = shared_balance + VacuumCostBalance;
> +
> /* also compute the total local balance */
> + local_balance = VacuumCostBalanceLocal + VacuumCostBalance;
> +
> +
> if ((new_balance >= VacuumCostLimit) &&
> + (local_balance > 0.5 * (VacuumCostLimit / nworkers)))
> + {
> +
> /* compute sleep time based on the local cost balance */
> + msec = VacuumCostDelay *
> VacuumCostBalanceLocal / VacuumCostLimit;
> + new_balance = shared_balance - VacuumCostBalanceLocal;
> +
> VacuumCostBalanceLocal = 0;
> + }
> +
> + if (pg_atomic_compare_exchange_u32(VacuumSharedCostBalance,
> +
>   _balance,
> +
>   new_balance))
> + {
> + /* Updated successfully, break */
> +
> break;
> + }
> + }
> +
> + VacuumCostBalanceLocal += VacuumCostBalance;
>
> I see multiple problems with this code. (a) if the VacuumSharedCostBalance is 
> changed by the time of compare and exchange, then the next iteration might 
> not compute the correct values as you might have reset VacuumCostBalanceLocal 
> by that time. (b) In code line, new_balance = shared_balance - 
> VacuumCostBalanceLocal, you need to use new_balance instead of 
> shared_balance, otherwise, it won't account for the balance of the latest 
> cycle.  (c) In code line, msec = VacuumCostDelay * VacuumCostBalanceLocal / 
> VacuumCostLimit;, I think you need to use local_balance for reasons similar 
> to (b). (d) I think we can write this code with a lesser number of variables.

In your code, I think if two workers enter to compute_parallel_delay
function at the same time, they add their local balance to
VacuumSharedCostBalance and both workers sleep because both values
reach the VacuumCostLimit. But either one worker should not sleep in
this case.

>
> I have fixed all these problems and used a slightly different way to compute 
> the parallel delay.  See compute_parallel_delay() in the attached delta patch.
>
> 2.
> + /* Setup the shared cost-based vacuum delay and launch workers*/
> + if (nworkers > 0)
> + {
> + /*
> + * Reset the local value so that we compute cost balance during
> + * parallel index vacuuming.
> + */
> + VacuumCostBalance = 0;
> + VacuumCostBalanceLocal = 0;
> +
> + LaunchParallelWorkers(lps->pcxt, nworkers);
> +
> + /* Enable shared costing iff we process indexes in parallel. */
> + if (lps->pcxt->nworkers_launched > 0)
> + {
> + /* Enable shared cost balance */
> + VacuumSharedCostBalance = &(lps->lvshared->cost_balance);
> + VacuumActiveNWorkers = &(lps->lvshared->active_nworkers);
> +
> + /*
> + * Set up shared cost balance and the number of active workers for
> + * vacuum delay.
> + */
> + pg_atomic_write_u32(VacuumSharedCostBalance, VacuumCostBalance);
> + pg_atomic_write_u32(VacuumActiveNWorkers, 0);
>
> This code has issues. 

Re: [HACKERS] [WIP] Effective storage of duplicates in B-tree index.

[Peter Geoghegan]

On Tue, Nov 12, 2019 at 3:21 PM Peter Geoghegan  wrote:
> * Decided to go back to turning deduplication on by default with
> non-unique indexes, and off by default using unique indexes.
>
> The unique index stuff was regressed enough with INSERT-heavy
> workloads that I was put off, despite my initial enthusiasm for
> enabling deduplication everywhere.

I have changed my mind about this again. I now think that it would
make sense to treat deduplication within unique indexes as a separate
feature that cannot be disabled by the GUC at all (though we'd
probably still respect the storage parameter for debugging purposes).
I have found that fixing the WAL record size issue has helped remove
what looked like a performance penalty for deduplication (but was
actually just a general regression). Also, I have found a way of
selectively applying deduplication within unique indexes that seems to
have no downside, and considerable upside.

The new criteria/heuristic for unique indexes is very simple: If a
unique index has an existing item that is a duplicate on the incoming
item at the point that we might have to split the page, then apply
deduplication. Otherwise (when the incoming item has no duplicates),
don't apply deduplication at all -- just accept that we'll have to
split the page. We already cache the bounds of our initial binary
search in insert state, so we can reuse that information within
_bt_findinsertloc() when considering deduplication in unique indexes.

This heuristic makes sense because deduplication within unique indexes
should only target leaf pages that cannot possibly receive new values.
In many cases, the only reason why almost all primary key leaf pages
can ever split is because of non-HOT updates whose new HOT chain needs
a new, equal entry in the primary key. This is the case with your
standard identity column/serial primary key, for example (only the
rightmost page will have a page split due to the insertion of new
logical rows -- everything other variety of page split must be due to
new physical tuples/versions). I imagine that it is possible for a
leaf page to be a "mixture"  of these two basic/general tendencies,
but not for long. It really doesn't matter if we occasionally fail to
delay a page split where that was possible, nor does it matter if we
occasionally apply deduplication when that won't delay a split for
very long -- pretty soon the page will split anyway. A split ought to
separate the parts of the keyspace that exhibit each tendency. In
general, we're only interested in delaying page splits in unique
indexes *indefinitely*, since in effect that will prevent them
*entirely*. (So the goal is *significantly* different to our general
goal for deduplication -- it's about buying time for VACUUM to run or
whatever, rather than buying space.)

This heuristic helps the TPC-C "old order" tables PK from bloating
quite noticeably, since that was the only unique index that is really
affected by non-HOT UPDATEs (i.e. the UPDATE queries that touch that
table happen to not be HOT-safe in general, which is not the case for
any other table). It doesn't regress anything else from TPC-C, since
there really isn't a benefit for other tables. More importantly, the
working/draft version of the patch will often avoid a huge amount of
bloat in a pgbench-style workload that has an extra index on the
pgbench_accounts table, to prevent HOT updates. The accounts primary
key (pgbench_accounts_pkey) hardly grows at all with the patch, but
grows 2x on master.

This 2x space saving seems to occur reliably, unless there is a lot of
contention on individual *pages*, in which case the bloat can be
delayed but not prevented. We get that 2x space saving with either
uniformly distributed random updates on pgbench_accounts (i.e. the
pgbench default), or with a skewed distribution that hashes the PRNG's
value. Hashing like this simulates a workload where there the skew
isn't concentrated in one part of the key space (i.e. there is skew,
but very popular values are scattered throughout the index evenly,
rather than being concentrated together in just a few leaf pages).

Can anyone think of an adversarial case, that we may not do so well on
with the new "only deduplicate within unique indexes when new item
already has a duplicate" strategy? I'm having difficulty identifying
some kind of worst case.

-- 
Peter Geoghegan

Re: [PATCH] Addition of JetBrains project directory to .gitignore

[David Nedrow]

Got it, and that makes sense.

I hereby withdraw this patch. ;)

- David

> On Dec 3, 2019, at 10:08, Tom Lane  wrote:
> 
> Daniel Gustafsson  writes:
 On 3 Dec 2019, at 15:47, David Nedrow  wrote:
>>> This patch simply adds “.idea/“ to the list of global excludes across all 
>>> subdirectories. This directory is created when a JetBrains IDE is used to 
>>> open a project. In my specific case, Clion is creating the project 
>>> directory.
>>> 
>>> The ONLY change in the patch is the “.idea/“ addition to .gitignore.
> 
>> -1.  This seems like something better suited in a local gitignore for those 
>> who
>> use Jetbrains products. See the documentation for ~/.gitignore_global.
> 
> Yeah, we already have a policy that we won't add entries for, say,
> editor backup files.  This seems like the same thing.  It's stuff
> generated by a tool you use, and you'd need it for any project
> you work on, so a personal ~/.gitexclude seems like the answer.
> 
> (Roughly speaking, I think the project policy is/should be that only
> junk files created by application of build rules in our Makefiles
> should be excluded by our own .gitexclude files.)
> 
> As a point of reference, I have
> 
> $ cat ~/.gitexclude 
> *~
> *.orig
> 
> to suppress emacs backup files and patch backup files respectively.
> Somebody who prefers another editor would have no use for *~.
> 
>regards, tom lane

Setting min/max TLS protocol in clientside libpq

[Daniel Gustafsson]

Responding to the recent thread on bumping the default TLS version, I realized
that we don't have a way to set the minimum/maximum TLS protocol version in
clientside libpq.  Setting the maximum protocol version obviously not terribly
important (possibly with the exception of misbehaving middle-boxes and
testing), but the minimum version can be quite useful to avoid misbehaving
and/or misconfigured servers etc.

The attached patch implements two new connection string variables for minimum
and maximum TLS protocol version, mimicking how it's done in the backend.  This
does duplicate a bit of code from be-secure-openssl.c to cope with older
versions of OpenSSL, but it seemed a too trivial duplication to create
common/openssl.c (but others might disagree).

This can today be achieved by editing the local openssl configuration, but
having an override in libpq to tighten down the connection parameters make it
far easier for the user/application IMO.

cheers ./daniel



libpq_minmaxproto.patch
Description: Binary data

Re: Bogus EXPLAIN results with column aliases for mismatched partitions

[Tom Lane]

Etsuro Fujita  writes:
> On Tue, Dec 3, 2019 at 6:45 AM Tom Lane  wrote:
>> Concretely, I'm thinking of the attached (on top of the other patch,
>> which I just pushed).  This agrees exactly with what ExplainTargetRel
>> does for regular scans.

> Thanks for the patch!  (The patch didn't apply to HEAD cleanly,
> though.)  I like consistency, so +1 for the change.

Yeah, 55a1954da probably changed the expected output from what that
has.  I'll clean it up and push.

>> One could make an argument that we should schema-qualify, regardless
>> of the "verbose" flag, if the output format isn't EXPLAIN_FORMAT_TEXT.
>> That would reduce the amount of variability that plan analysis tools
>> have to cope with.  However, ExplainTargetRel itself doesn't provide
>> the schema in non-verbose mode.  Maybe it should, ie we should change
>> it like ...

> Seems like another issue to me.

Agreed.  When/if we change that, we could make this code follow along.

regards, tom lane

Re: Windows buildfarm members vs. new async-notify isolation test

[Tom Lane]

Mark Dilger  writes:
> On 12/2/19 11:42 AM, Andrew Dunstan wrote:
>> On 12/2/19 11:23 AM, Tom Lane wrote:
>>> I'm a little baffled as to what this might be --- some sort of
>>> timing problem in our Windows signal emulation, perhaps?  But
>>> if so, why haven't we found it years ago?

> I would be curious to see if there is a race condition in
> src/test/isolation/isolationtester.c between the loop starting
> on line 820:
>while ((res = PQgetResult(conn)))
>{
>   ...
>}
> and the attempt to consume input that might include NOTIFY
> messages on line 861:
>PQconsumeInput(conn);

In principle, the issue should not be there, because commits
790026972 et al should have ensured that the NOTIFY protocol
message comes out before ReadyForQuery (and thus, libpq will
absorb it before PQgetResult will return NULL).  I think the
timing problem --- if that's what it is --- must be on the
backend side; somehow the backend is not processing the
inbound notify queue before it goes idle.

Hmm ... just looking at the code again, could it be that there's
no well-placed CHECK_FOR_INTERRUPTS?  Andrew, could you see if
injecting one in what 790026972 added to postgres.c helps?
That is,

/*
 * Also process incoming notifies, if any.  This is mostly to
 * ensure stable behavior in tests: if any notifies were
 * received during the just-finished transaction, they'll be
 * seen by the client before ReadyForQuery is.
 */
+   CHECK_FOR_INTERRUPTS();
if (notifyInterruptPending)
ProcessNotifyInterrupt();


regards, tom lane

Re: Using XLogFileNameP in critical section

[Tom Lane]

I wrote:
> Also, buildfarm member drongo is not happy:
> postgres.def : error LNK2001: unresolved external symbol XLogFileNameP 
> [C:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj]
> Release/postgres/postgres.lib : fatal error LNK1120: 1 unresolved externals 
> [C:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj]
> I'm guessing you missed a reference someplace.

Hm ... grep swears up and down that there is no remaining instance
of the string "XLogFileNameP" anywhere in the tree.  So this doesn't
seem to be the fault of 9989d37d1 per se.  What my eye now falls on
is this, a bit further up in the build log [1]:

...
PreLinkEvent:
  Generate DEF file
  perl src\tools\msvc\gendef.pl Release\postgres x64
  :VCEnd
  Not re-generating POSTGRES.DEF, file already exists.
Link:
...

So it seems that the problem might really be a faulty rule in our
MSVC build script about when postgres.def needs to be regenerated?
Or else it's some weird caching problem on drongo --- the lack of
complaints from other Windows critters might point the finger
that way.

regards, tom lane

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo=2019-12-03%2007%3A30%3A01

Re: pgbench -i progress output on terminal

[Fabien COELHO]

Attached v4.


Patch applies cleanly, compiles, works for me. Put it back to ready.

--
Fabien.

Re: Using XLogFileNameP in critical section

[Tom Lane]

Alvaro Herrera  writes:
> I'm not sure that the internationalization stuff in issue_xlog_fsync is
> correct.  I think the _() should be gettext_noop(), or alternatively the
> errmsg() should be errmsg_internal(); otherwise the translation is
> invoked twice.  (I didn't verify this.)

Also, buildfarm member drongo is not happy:

postgres.def : error LNK2001: unresolved external symbol XLogFileNameP 
[C:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj]
Release/postgres/postgres.lib : fatal error LNK1120: 1 unresolved externals 
[C:\prog\bf\root\HEAD\pgsql.build\postgres.vcxproj]

I'm guessing you missed a reference someplace.

regards, tom lane

Re: log bind parameter values on error

[Alvaro Herrera]

On 2019-Sep-20, Andres Freund wrote:

> > > > +   appendStringInfoCharMacro(_str, '\'');
> > > > +   for (p = pstring; *p; p++)
> > > > +   {
> > > > +   if (*p == '\'') /* double single quotes */
> > > > +   appendStringInfoCharMacro(_str, 
> > > > *p);
> > > > +   appendStringInfoCharMacro(_str, *p);
> > > > +   }
> > > > +   appendStringInfoCharMacro(_str, '\'');
> > > 
> > > I know this is old code, but: This is really inefficient. Will cause a
> > > lot of unnecessary memory-reallocations for large text outputs, because
> > > we don't immediately grow to at least close to the required size.

> I'd probably just count the ' in one pass, enlarge the stringinfo to the
> required size, and then put the string directly into he stringbuffer.
> Possibly just putting the necessary code into stringinfo.c. We already
> have multiple copies of this inefficient logic...

I stole Alexey's code for this from downthread and tried to apply to all
these places.  However, I did not put it to use in all these places you
mention, because each of them has slightly different requirements;
details below.

Now, I have to say that this doesn't make me terribly happy, because I
would like the additional ability to limit the printed values to N
bytes.  This means the new function would have to have an additional
argument to indicate the maximum length (pass 0 to print all args
whole) ... and the logic then because more involved because we need
logic to stop printing early.

I think the current callers should all use the early termination logic;
having plpgsql potentially produce 1 MB of log output because of a long
argument seems silly.  So I see no use for this function *without* the
length-limiting logic.

> But even if not, I don't think writing data into the stringbuf directly
> is that ugly, we do that in enough places that I'd argue that that's
> basically part of how it's expected to be used.
> 
> In HEAD there's at least
> - postgres.c:errdetail_params(),
> - pl_exec.c:format_preparedparamsdata()
>   pl_exec.c:format_expr_params()

These are changed in the patch; they're all alike.

> - dblink.c:escape_param_str()

This one wants to use \' and \\ instead of just doubling each '.
The resulting string is used as libpq conninfo, so using doubled ' does
not work.

> - deparse.c:deparseStringLiteral()

This one wants to use E''-style strings that ignore std_conforming_strings;
the logic would need to detect two chars ' and \ instead of just ' so
we'd need to use strcspn instead of strchr(); that seems a lot slower.

> - xlog.c:do_pg_start_backup() (after "Add the escape character"),

This one wants to escape \n chars.

> - tsearchcmds.c:serialize_deflist()

This wants E''-style strings, like deparse.c.

> - ruleutils.c:simple_quote_literal()

Produce an escaped string according to the prevailing
std_conforming_strings.


I think it's possible to produce a function that would satisfy all of
these, but it would be another function similar to the one I'm writing
here, not the same one.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
ggPostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 99cf1d122bc935a4060c6b359fb2c262035039df Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Tue, 3 Dec 2019 10:08:35 -0300
Subject: [PATCH] Add appendStringInfoStringQuoted
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Simplifies some coding that prints parameters, as well as optimize to do
it per non-quote chunks instead of per byte.

Author: Alexey Bashtanov and Álvaro Herrera, after a suggestion from Andres Freund
Discussion: https://postgr.es/m/20190920203905.xkv5udsd5dxfs...@alap3.anarazel.de
---
 src/backend/tcop/postgres.c  | 10 +
 src/common/stringinfo.c  | 40 
 src/include/lib/stringinfo.h |  7 +++
 src/pl/plpgsql/src/pl_exec.c | 34 --
 4 files changed, 56 insertions(+), 35 deletions(-)

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index 3b85e48333..0bff20ad67 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2348,7 +2348,6 @@ errdetail_params(ParamListInfo params)
 			Oid			typoutput;
 			bool		typisvarlena;
 			char	   *pstring;
-			char	   *p;
 
 			appendStringInfo(_str, "%s$%d = ",
 			 paramno > 0 ? ", " : "",
@@ -2364,14 +2363,7 @@ errdetail_params(ParamListInfo params)
 
 			pstring = OidOutputFunctionCall(typoutput, prm->value);
 
-			appendStringInfoCharMacro(_str, '\'');
-			for (p = pstring; *p; p++)
-			{
-if (*p == '\'') /* double single quotes */
-	appendStringInfoCharMacro(_str, *p);
-appendStringInfoCharMacro(_str, *p);
-			}
-			appendStringInfoCharMacro(_str, '\'');
+			appendStringInfoStringQuoted(_str, pstring);
 
 			pfree(pstring);
 		}
diff --git 

Re: Allow relocatable extension to use @extschema@?

[Tom Lane]

Alexander Korotkov  writes:
> But nevertheless should we allow relocatable extension to use
> @extschema@.  Any thoughts?

No.  The reasoning in the comment still holds good: if you embed
@extschema@ in an object's definition, it becomes nonrelocatable.

regards, tom lane

Re: Windows buildfarm members vs. new async-notify isolation test

[Mark Dilger]

On 12/2/19 11:42 AM, Andrew Dunstan wrote:


On 12/2/19 11:23 AM, Tom Lane wrote:

I see from the buildfarm status page that since commits 6b802cfc7
et al went in a week ago, frogmouth and currawong have failed that
new test case every time, with the symptom

== pgsql.build/src/test/isolation/regression.diffs 
===
*** 
c:/prog/bf/root/REL_10_STABLE/pgsql.build/src/test/isolation/expected/async-notify.out
  Mon Nov 25 00:30:49 2019
--- 
c:/prog/bf/root/REL_10_STABLE/pgsql.build/src/test/isolation/results/async-notify.out
   Mon Dec  2 00:54:26 2019
***
*** 93,99 
   step llisten: LISTEN c1; LISTEN c2;
   step lcommit: COMMIT;
   step l2commit: COMMIT;
- listener2: NOTIFY "c1" with payload "" from notifier
   step l2stop: UNLISTEN *;
   
   starting permutation: llisten lbegin usage bignotify usage

--- 93,98 

(Note that these two critters don't run branches v11 and up, which
is why they're only showing this failure in 10 and 9.6.)

drongo showed the same failure once in v10, and fairywren showed
it once in v12.  Every other buildfarm animal seems happy.

I'm a little baffled as to what this might be --- some sort of
timing problem in our Windows signal emulation, perhaps?  But
if so, why haven't we found it years ago?

I don't have any ability to test this myself, so would appreciate
help or ideas.




I can test things, but I don't really know what to test. FYI frogmouth
and currawong run on virtualized XP. drongo anf fairywrne run on
virtualized WS2019. Neither VM is heavily resourced.


Hi Andrew, if you have time you could perhaps check the
isolation test structure itself.  Like Tom, I don't have a
Windows box to test this.

I would be curious to see if there is a race condition in
src/test/isolation/isolationtester.c between the loop starting
on line 820:

  while ((res = PQgetResult(conn)))
  {
 ...
  }

and the attempt to consume input that might include NOTIFY
messages on line 861:

  PQconsumeInput(conn);

If the first loop consumes the commit message, gets no
further PGresult from PQgetResult, and finishes, and execution
proceeds to PQconsumeInput before the NOTIFY has arrived
over the socket, there won't be anything for PQnotifies to
return, and hence for try_complete_step to print before
returning.

I'm not sure if it is possible for the commit message to
arrive before the notify message in the fashion I am describing,
but that's something you might easily check by having
isolationtester sleep before PQconsumeInput on line 861.


--
Mark Dilger

Re: [PATCH] Addition of JetBrains project directory to .gitignore

[Tom Lane]

Daniel Gustafsson  writes:
>> On 3 Dec 2019, at 15:47, David Nedrow  wrote:
>> This patch simply adds “.idea/“ to the list of global excludes across all 
>> subdirectories. This directory is created when a JetBrains IDE is used to 
>> open a project. In my specific case, Clion is creating the project directory.
>> 
>> The ONLY change in the patch is the “.idea/“ addition to .gitignore.

> -1.  This seems like something better suited in a local gitignore for those 
> who
> use Jetbrains products. See the documentation for ~/.gitignore_global.

Yeah, we already have a policy that we won't add entries for, say,
editor backup files.  This seems like the same thing.  It's stuff
generated by a tool you use, and you'd need it for any project
you work on, so a personal ~/.gitexclude seems like the answer.

(Roughly speaking, I think the project policy is/should be that only
junk files created by application of build rules in our Makefiles
should be excluded by our own .gitexclude files.)

As a point of reference, I have

$ cat ~/.gitexclude 
*~
*.orig

to suppress emacs backup files and patch backup files respectively.
Somebody who prefers another editor would have no use for *~.

regards, tom lane

Session WAL activity

[Konstantin Knizhnik]

Hi hackers,

One of our customers complains about that some sessions generates "too 
much WAL records".
Certainly WAL activity doesn't indicate a problem itself: huge workload 
cause huge WAL activity.
But them are trying to understand which clients produces so much 
database changes and complain that there is
no way to get such information in Postgres. For example in Oracle this 
problems can be solved in this way:


http://www.dba-oracle.com/t_find_session_generating_high_redo.htm

Unfortunately there is actually no simple and accurate way to calculate 
amount of WAL produced by the particular session.
It is possible to parse WAL (for example using pg_waldump), then using 
XID->pid mapping accumulate size of transactions produced by each backend.

But this is very inconvenient and not DBA friendly approach.

I have implemented small patch which collects such statistic.
I have added walWritten  field to PGPROC and increment it in 
CopyXLogRecordToWAL.
It is possible to inspect this field using pg_stat_get_wal_activity(pid) 
function and also I have added
pg_stat_wal_activity which just adds  wal_written to standard 
pg_activity view:


postgres=# select pid, backend_type, wal_written from pg_stat_wal_activity ;
 pid  | backend_type | wal_written
--+--+-
 4405 | autovacuum launcher  |   0
 4407 | logical replication launcher |   0
 4750 | client backend   |   86195
 4403 | background writer| 204
 4402 | checkpointer | 328
 4404 | walwriter|   0
(6 rows)



I wonder if such extra statistic about session WAL activity is 
considered to be useful?
The only problem with this approach from my point of view is adding 8 
bytes to PGPROC.
But there are already so many fields in this structure 
(sizeof(PGPROC)=816), that adding yet another 8 bytes should not be 
noticeable.


Comments are welcome.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 5f0ee50..c985b04 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -1486,6 +1486,9 @@ CopyXLogRecordToWAL(int write_len, bool isLogSwitch, XLogRecData *rdata,
 	currpos = GetXLogBuffer(CurrPos);
 	freespace = INSERT_FREESPACE(CurrPos);
 
+	/* Accumulate amount of data written to WAL for pg_xact_activity */
+	MyProc->walWritten += write_len;
+
 	/*
 	 * there should be enough space for at least the first field (xl_tot_len)
 	 * on this page.
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 000cff3..037d313 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -758,6 +758,9 @@ CREATE VIEW pg_stat_activity AS
 LEFT JOIN pg_database AS D ON (S.datid = D.oid)
 LEFT JOIN pg_authid AS U ON (S.usesysid = U.oid);
 
+CREATE VIEW pg_stat_wal_activity AS
+SELECT a.*,pg_stat_get_wal_activity(a.pid) as wal_written FROM pg_stat_activity a;
+
 CREATE VIEW pg_stat_replication AS
 SELECT
 S.pid,
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index fff0628..bf6ab34 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -390,6 +390,8 @@ InitProcess(void)
 	MyPgXact->xid = InvalidTransactionId;
 	MyPgXact->xmin = InvalidTransactionId;
 	MyProc->pid = MyProcPid;
+	MyProc->walWritten = 0;
+
 	/* backendId, databaseId and roleId will be filled in later */
 	MyProc->backendId = InvalidBackendId;
 	MyProc->databaseId = InvalidOid;
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 05240bf..b3acba5 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -546,7 +546,8 @@ pg_stat_get_progress_info(PG_FUNCTION_ARGS)
 Datum
 pg_stat_get_activity(PG_FUNCTION_ARGS)
 {
-#define PG_STAT_GET_ACTIVITY_COLS	29
+
+	#define PG_STAT_GET_ACTIVITY_COLS	29
 	int			num_backends = pgstat_fetch_stat_numbackends();
 	int			curr_backend;
 	int			pid = PG_ARGISNULL(0) ? -1 : PG_GETARG_INT32(0);
@@ -919,6 +920,22 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 
 
 Datum
+pg_stat_get_wal_activity(PG_FUNCTION_ARGS)
+{
+	int32	pid = PG_GETARG_INT32(0);
+	PGPROC* proc = BackendPidGetProc(pid);
+	if (proc == NULL)
+	{
+		proc = AuxiliaryPidGetProc(pid);
+	}
+	if (proc == NULL)
+		PG_RETURN_NULL();
+	else
+		PG_RETURN_INT64(proc->walWritten);
+}
+
+
+Datum
 pg_backend_pid(PG_FUNCTION_ARGS)
 {
 	PG_RETURN_INT32(MyProcPid);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index b759b15..6d31afd 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -5152,6 +5152,10 @@
   proargmodes => '{i,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}',
   proargnames 

Re: [PATCH] Addition of JetBrains project directory to .gitignore

[Daniel Gustafsson]

> On 3 Dec 2019, at 15:56, David Nedrow  wrote: 

> Hmmm. I can see that. However, there are already entries for Microsoft Visual 
> C++ at the global level. Wouldn’t this fall into the same category?

Not really, the files in the current .gitignore are artifacts of the build-
system which is provided by the postgres tree (MSVC building, gcov etc); there
are no editor specific files ignored there.

cheers ./daniel

Re: [PATCH] Addition of JetBrains project directory to .gitignore

[Daniel Gustafsson]

> On 3 Dec 2019, at 15:47, David Nedrow  wrote:

> This patch simply adds “.idea/“ to the list of global excludes across all 
> subdirectories. This directory is created when a JetBrains IDE is used to 
> open a project. In my specific case, Clion is creating the project directory.
> 
> The ONLY change in the patch is the “.idea/“ addition to .gitignore.

-1.  This seems like something better suited in a local gitignore for those who
use Jetbrains products. See the documentation for ~/.gitignore_global.

cheers ./daniel

[PATCH] Addition of JetBrains project directory to .gitignore

[David Nedrow]

This patch simply adds “.idea/“ to the list of global excludes across all 
subdirectories. This directory is created when a JetBrains IDE is used to open 
a project. In my specific case, Clion is creating the project directory.

The ONLY change in the patch is the “.idea/“ addition to .gitignore.

David Nedrow
dned...@me.com




gitignore-JetBrains-v1.patch
Description: Binary data

Re: Allow superuser to grant passwordless connection rights on postgres_fdw

[Stephen Frost]

Greetings,

* Craig Ringer (cr...@2ndquadrant.com) wrote:
> On Mon, 4 Nov 2019 at 12:20, Stephen Frost  wrote:
> > I've long felt that the way to handle this kind of requirement is to
> > have a "trusted remote server" kind of option- where the local server
> > authenticates to the remote server as a *server* and then says "this is
> > the user on this server, and this is the user that this user wishes to
> > be" and the remote server is then able to decide if they accept that, or
> > not.
> 
> The original use case for the patch was to allow FDWs to use SSL/TLS client
> certificates. Each user-mapping has its own certificate - there's a
> separate patch to allow that. So there's no delegation of trust via
> Kerberos etc in that particular case.
> 
> I can see value in using Kerberos etc for that too though, as it separates
> authorization and authentication in the same manner as most sensible
> systems. You can say "user postgres@foo is trusted to vet users so you can
> safely hand out tickets for any bar@foo that postgres@foo says is legit".

So, just to be clear, the way this *actually* works is a bit different
from the way being described above, last time I looked into Kerberos
delegations anyway.

Essentially, the KDC can be set up to allow 'bar@foo' to request a
ticket to delegate to 'postgres@foo', which then allows 'postgres@foo'
to connect as if they are 'bar@foo' to some other service (and in some
implementations, I believe it's further possible to say that the ticket
for 'bar@foo' which is delegated to 'postgres@foo' is only allowed to
request tickets for certain specific services, such as 'postgres2@foo'
or what-have-you).

Note that setting this up with an MIT KDC requires configuring it with
an LDAP backend as the traditional KDC database doesn't support this
kind of complex delegation control (again, last time I checked anyway).

> I would strongly discourage allowing all users on host A to authenticate as
> user postgres on host B. But with appropriate user-mappings support, we
> could likely support that sort of model for both SSPI and Kerberos.

Ideally, both sides would get a 'vote' regarding what's allowed, I would
think.  That is, the connecting side would have to have a user mapping
that says "this authenticated user is allowed to connect to this remote
server as this user", and the remote server would have something like
"this server that's connecting, validated by the certificate presented
by the server, is allowed to authenticate as this user".  I feel like
we're mostly there by allowing the connecting server to use a
certificate to connect to the remote server, while it's also checking
the user mapping, and the remote server's pg_hba.conf being configured
to allow cert-based auth with a CN mapping from the CN of the connecting
server's certificate to authenticate to whatever users the remote server
wants to allow.  Is that more-or-less the idea here..?

> A necessary prerequisite is that Pg be able to cope with passwordless
> user-mappings though. Hence this patch.

Sure, that part seems like it makes sense to me (and perhaps has now
been done, just catching up on things after travel and holidays and such
here in the US).

Thanks!

Stephen


signature.asc
Description: PGP signature

Minor comment fixes for instrumentation.h

[Rafia Sabih]

Hello,

While going through this file I noticed some inconsistencies in the
comments. Please find attachment for the fix.

-- 
Regards,
Rafia Sabih
diff --git a/src/include/executor/instrument.h b/src/include/executor/instrument.h
index 70d8632305..13aaf05453 100644
--- a/src/include/executor/instrument.h
+++ b/src/include/executor/instrument.h
@@ -48,20 +48,20 @@ typedef struct Instrumentation
 	bool		need_bufusage;	/* true if we need buffer usage data */
 	/* Info about current plan cycle: */
 	bool		running;		/* true if we've completed first tuple */
-	instr_time	starttime;		/* Start time of current iteration of node */
-	instr_time	counter;		/* Accumulated runtime for this node */
-	double		firsttuple;		/* Time for first tuple of this cycle */
-	double		tuplecount;		/* Tuples emitted so far this cycle */
-	BufferUsage bufusage_start; /* Buffer usage at start */
+	instr_time	starttime;		/* start time of current iteration of node */
+	instr_time	counter;		/* accumulated runtime for this node */
+	double		firsttuple;		/* time for first tuple of this cycle */
+	double		tuplecount;		/* # of tuples emitted so far this cycle */
+	BufferUsage bufusage_start; /* buffer usage at start */
 	/* Accumulated statistics across all completed cycles: */
-	double		startup;		/* Total startup time (in seconds) */
-	double		total;			/* Total total time (in seconds) */
-	double		ntuples;		/* Total tuples produced */
-	double		ntuples2;		/* Secondary node-specific tuple counter */
+	double		startup;		/* total startup time (in seconds) */
+	double		total;			/* total time (in seconds) */
+	double		ntuples;		/* total tuples produced */
+	double		ntuples2;		/* secondary node-specific tuple counter */
 	double		nloops;			/* # of run cycles for this node */
-	double		nfiltered1;		/* # tuples removed by scanqual or joinqual */
-	double		nfiltered2;		/* # tuples removed by "other" quals */
-	BufferUsage bufusage;		/* Total buffer usage */
+	double		nfiltered1;		/* # of tuples removed by scanqual or joinqual */
+	double		nfiltered2;		/* # of tuples removed by "other" quals */
+	BufferUsage bufusage;		/* total buffer usage */
 } Instrumentation;
 
 typedef struct WorkerInstrumentation

Re: Using XLogFileNameP in critical section

[Alvaro Herrera]

On 2019-Dec-03, Michael Paquier wrote:

> Per the low probability of the failures, I did not backpatch that
> stuff.  I quickly looked at applying that further down, and attached
> is a version for v12 FWIW, and I suspect much more conflicts the more
> you go down (wal segment size added in 11, different code paths for
> replication, etc.).

You didn't attach anything, but I concur about the low probability
aspect: the assertion failure does not occur in production builds
(obviously); and only an out-of-memory situation is a real problem when
an fsync fails.  Anyway this should be a very localized fix, right?

I'm not sure that the internationalization stuff in issue_xlog_fsync is
correct.  I think the _() should be gettext_noop(), or alternatively the
errmsg() should be errmsg_internal(); otherwise the translation is
invoked twice.  (I didn't verify this.)

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

Re: fe-utils - share query cancellation code

[Fabien COELHO]

Bonjour Michaël,

Committed the patch after splitting things into two commits and after 
testing things from Linux and from a Windows console: the actual 
refactoring and the pgbench changes.


I have found that we have a useless declaration of CancelRequested in 
common.h, which is already part of cancel.h.


Ok.

On top of that I think that we need to rework a bit the header 
inclusions of bin/scripts/, as per the attached.


Looks fine to me: patch applies, compiles, runs.

--
Fabien.

Allow relocatable extension to use @extschema@?

[Alexander Korotkov]

Hi!

During work on knn-btree patchset we've faced the need to move
functions/operators from contrib to core [1].  In the extension
upgrade script we need to use @extschema@ in order to distinguish
contrib and core objects.  However, it appears to be possible to use
@extschema@ only in non-relocatable extensions.  Comment in
extension.c says: "For a relocatable extension, we needn't do this.
There cannot be any need for @extschema@, else it wouldn't be
relocatable.".  I've explored that we've marked extension as
non-relocatable solely to use @extschema@ in script before [2].

So, it appears that comment in extension.c isn't true.  There is at
least two precedents when relocatable extension needs to use
@extschema@.  We've marked possibly relocatable extension as
non-relocatable once.  And we could do it at the second time.
Extension relocatability doesn't seem to me much value to sacrifice.
But nevertheless should we allow relocatable extension to use
@extschema@.  Any thoughts?

Links.
1. 
https://www.postgresql.org/message-id/CAPpHfdsWsb9T1eHdX%2Br7wnXbGJKQxSffc8gTGp4ZA2ewP49Hog%40mail.gmail.com
2. 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=de623f33353c96657651f9c3a6c8756616c610e4;hp=0024e348989254d48dc4afe9beab98a6994a791e

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] Block level parallel vacuum

[Mahendra Singh]

On Tue, 3 Dec 2019 at 16:27, Amit Kapila  wrote:

> On Tue, Dec 3, 2019 at 4:25 PM Amit Kapila 
> wrote:
>
>> Few other things, I would like you to consider.
>> 1.  I think disable_parallel_leader_participation related code can be
>> extracted into a separate patch as it is mainly a debug/test aid.  You can
>> also fix the problem reported by Mahendra in that context.
>>
>> 2. I think if we cam somehow disallow very small indexes to use parallel
>> workers, then it will be better.   Can we use  min_parallel_index_scan_size
>> to decide whether a particular index can participate in a parallel vacuum?
>>
>
> Forgot one minor point.  Please run pgindent on all the patches.
>

While reviewing and testing v35 patch set, I noticed some problems. Below
are some comments:

1.
  /*
+ * Since parallel workers cannot access data in temporary tables, parallel
+ * vacuum is not allowed for temporary relation. However rather than
+ * skipping vacuum on the table, just disabling parallel option is better
+ * option in most cases.
+ */
+ if (RelationUsesLocalBuffers(onerel) && params->nworkers >= 0)
+ {
+ ereport(WARNING,
+ (errmsg("disabling parallel option of vacuum on \"%s\" --- cannot vacuum
temporary tables in parallel",
+ RelationGetRelationName(onerel;
+ params->nworkers = 0;
+ }

Here, I think, we should set params->nworkers = -1 to disable parallel
vacuum for temporary tables. I noticed that even after warning, we were
doing vacuum in parallel mode and were launching parallel workers that was
wrong.

2.
Amit suggested me to check time taken by vacuum.sql regression test.

vacuum   ... ok20684 ms  ---on the top
of v35 patch set
vacuum   ... ok 1020 ms   ---without v35
patch set

Here, we can see that time taken by vacuum test case is increased too much
due to parallel vacuum test cases so I will try to come with a small test
case.

Thanks and Regards
Mahendra Thalor
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] WAL logging problem in 9.4.3?

[Kyotaro Horiguchi]

Hello.

At Thu, 28 Nov 2019 17:23:19 -0500, Noah Misch  wrote in 
> On Thu, Nov 28, 2019 at 09:35:08PM +0900, Kyotaro Horiguchi wrote:
> > I measured the performance with the latest patch set.
> > 
> > > 1. Determine $DDL_COUNT, a number of DDL transactions that take about one
> > >minute when done via syncs.
> > > 2. Start "pgbench -rP1 --progress-timestamp -T180 -c10 -j10".
> > > 3. Wait 10s.
> > > 4. Start one DDL backend that runs $DDL_COUNT transactions.
> > > 5. Save DDL start timestamp, DDL end timestamp, and pgbench output.
> 
> If you have the raw data requested in (5), please share them here so folks
> have the option to reproduce your graphs and calculations.

Sorry, I forgot to attach the scripts. The raw data was vanished into
unstable connection and the steps was quite crude. I prioritized on
showing some numbers at the time. I revised the scripts into more
automated way and will take numbers again.

> > > 2. Start server with wal_level = replica (all other variables are not
> > > changed) then run the attached ./bench.sh
> > 
> > The bench.sh attachment was missing; please attach it.  Please give the 
> > output
> > of this command:
> > 
> >   select name, setting from pg_settings where setting <> boot_val;

(I intentionally show all the results..)
=# select name, setting from pg_settings where setting<> boot_val;
name|  setting   
+
 application_name   | psql
 archive_command| (disabled)
 client_encoding| UTF8
 data_directory_mode| 0700
 default_text_search_config | pg_catalog.english
 lc_collate | en_US.UTF-8
 lc_ctype   | en_US.UTF-8
 lc_messages| en_US.UTF-8
 lc_monetary| en_US.UTF-8
 lc_numeric | en_US.UTF-8
 lc_time| en_US.UTF-8
 log_checkpoints| on
 log_file_mode  | 0600
 log_timezone   | Asia/Tokyo
 max_stack_depth| 2048
 max_wal_senders| 0
 max_wal_size   | 10240
 server_encoding| UTF8
 shared_buffers | 16384
 TimeZone   | Asia/Tokyo
 unix_socket_permissions| 0777
 wal_buffers| 512
 wal_level  | minimal
(23 rows)

The result for "replica" setting in the benchmark script are used as
base numbers (or the denominator of the percentages).

> > 3. Restart server with wal_level = replica then run the bench.sh
> > twice.
> 
> I assume this is wal_level=minimal, not wal_level=replica.

Oops! It's wrong I ran once with replica, then twice with minimal.


Anyway, I revised the benchmarking scripts and attached them.  The
parameters written in benchmain.sh were decided as ./bench2.pl 5
  s with wal_level=minimal server takes around 60
seconds.

I'll send the complete data tomorrow (in JST). The attached f.txt is
the result of preliminary test only with pages=100 and 250 (with HDD).

The attached files are:
  benchmain.sh- main script
  bench2.sh   - run a benchmark with a single set of parameters
  bench1.pl   - behchmark client program
  summarize.pl- script to summarize benchmain.sh's output
  f.txt.gz- result only for pages=100, DDL count = 2200 (not 2250)

How to run:

$ /..unpatched_path../initdb -D 
 (wal_level=replica, max_wal_senders=0, log_checkpoints=yes, max_wal_size=10GB)
$ /..patched_path../initdb -D 
 (wal_level=minimal, max_wal_senders=0, log_checkpoints=yes, max_wal_size=10GB)
$./benchmain.sh ># output raw data
$./summarize.pl [-v] <# show summary


With the attached f.txt, summarize.pl gives the following output.
WAL wins with the that pages.

$ cat f.txt | ./summarize.pl
## params: wal_level=replica mode=none pages=100 count=353 scale=20
(% are relative to "before")
before: tps  262.3 (100.0%), lat39.840 ms (100.0%) (29 samples)
during: tps  120.7 ( 46.0%), lat   112.508 ms (282.4%) (35 samples)
 after: tps  106.3 ( 40.5%), lat   163.492 ms (410.4%) (86 samples)
DDL time:  34883 ms ( 100.0% relative to mode=none)
## params: wal_level=minimal mode=sync pages=100 count=353 scale=20
(% are relative to "before")
before: tps  226.3 (100.0%), lat48.091 ms (100.0%) (29 samples)
during: tps   83.0 ( 36.7%), lat   184.942 ms (384.6%) (100 samples)
 after: tps   82.6 ( 36.5%), lat   196.863 ms (409.4%) (21 samples)
DDL time:  99239 ms ( 284.5% relative to mode=none)
## params: wal_level=minimal mode=WAL pages=100 count=353 scale=20
(% are relative to "before")
before: tps  240.3 (100.0%), lat44.686 ms (100.0%) (29 samples)
during: tps  129.6 ( 53.9%), lat   113.585 ms (254.2%) (31 samples)
 after: tps  124.5 ( 51.8%), lat   141.992 ms (317.8%) (90 samples)
DDL time:  30392 ms (  87.1% relative to mode=none)
## params: wal_level=replica mode=none pages=250 count=258 scale=20
(% are relative to "before")
before: tps  266.3 (100.0%), lat45.884 ms (100.0%) (29 samples)
during: 

Re: Update minimum SSL version

[Magnus Hagander]

On Tue, Dec 3, 2019 at 12:09 PM Michael Paquier  wrote:

> On Tue, Dec 03, 2019 at 10:10:57AM +0100, Magnus Hagander wrote:
> > Is 1.0.1 considered a separate major from 1.0.0, in this reasoning?
> Because
> > while retiring 1.0.0 should probably not be that terrible, 1.0.1 is still
> > in very widespread use on most long term supported distributions.
>
> 1.0.1 and 1.0.0 are two different major releases in the OpenSSL world,
> so my suggestion would be to cut support for everything which does not
> have TLSv1.2, meaning that we keep compatibility with 1.0.1 for
> a longer period.
>

Good, that's what I thought you meant :) And that makes it sound like a
working plan to me.

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

Re: Update minimum SSL version

[Michael Paquier]

On Tue, Dec 03, 2019 at 10:10:57AM +0100, Magnus Hagander wrote:
> Is 1.0.1 considered a separate major from 1.0.0, in this reasoning? Because
> while retiring 1.0.0 should probably not be that terrible, 1.0.1 is still
> in very widespread use on most long term supported distributions.

1.0.1 and 1.0.0 are two different major releases in the OpenSSL world,
so my suggestion would be to cut support for everything which does not
have TLSv1.2, meaning that we keep compatibility with 1.0.1 for
a longer period.
--
Michael


signature.asc
Description: PGP signature

Errors when update a view with conditional-INSTEAD rules

[Pengzhou Tang]

Hi Hackers,

I hit an error when updating a view with conditional INSTEAD OF rules, the
reproduce steps are list below:

CREATE TABLE t1(a int, b int);

CREATE TABLE t2(a int, b int);

CREATE VIEW v1 AS SELECT * FROM t1 where b > 100;

INSERT INTO v1 values(1, 110);

SELECT * FROM t1;


CREATE OR REPLACE rule r1 AS

ON UPDATE TO v1

WHERE old.a > new.b

DO INSTEAD (

INSERT INTO t2 values(old.a, old.b);

);


UPDATE v1 SET b = 2 WHERE a = 1;

*ERROR:  no relation entry for relid 2*


With some hacks, It is because, for conditional INSTEAD OF rules
 conditional, the original UPDATE operation also need to perform on the
view, however, we didn't rewrite the target view for any view with INSTEAD
rules.

There should be only two cases that you can skip the rewrite of target view:
1) the view has INSTEAD OF triggers on the operations, the operations will
be replaced by trigger-defined
2) the view has INSTEAD OF rules and it is non conditional rules, the
operations will be replaced by actions.

It should be a typo in commit a99c42f291421572aef2, there is a description
in documents:
"There is a catch if you try to use conditional rules
for complex view updates: there must be an unconditional
INSTEAD rule for each action you wish to allow on the view."

Commit a99c42f291421572aef2 explicitly change the description that the
restriction only applies to complex view, conditional INSTEAD rule should
work for a simple view.

I attached a patch to fix it, please take a look,

Thanks,
Pengzhou


0001-Rewrite-the-target-view-if-it-has-conditional-INSTEA.patch
Description: Binary data

Re: benchmarking Flex practices

[John Naylor]

On Tue, Nov 26, 2019 at 10:32 PM Tom Lane  wrote:

> I haven't looked closely at what ecpg does with the processed
> identifiers.  If it just spits them out as-is, a possible solution
> is to not do anything about de-escaping, but pass the sequence
> U&"..." (plus UESCAPE ... if any), just like that, on to the grammar
> as the value of the IDENT token.

It does pass them along as-is, so I did it that way.

In the attached v10, I've synced both ECPG and psql.

> * I haven't convinced myself either way as to whether it'd be
> better to factor out the code duplicated between the UIDENT
> and UCONST cases in base_yylex.

I chose to factor it out, since we have 2 versions of parser.c, and
this way was much easier to work with.

Some notes:

I arranged for the ECPG grammar to only see SCONST and IDENT. With
UCONST and UIDENT out of the way, it was a small additional step to
put all string reconstruction into the lexer, which has the advantage
of allowing removal of the other special-case ECPG string tokens as
well. The fewer special cases involved in pasting the grammar
together, the better. In doing so, I've probably introduced memory
leaks, but I wanted to get your opinion on the overall approach before
investigating.

In ECPG's parser.c, I simply copied check_uescapechar() and
ecpg_isspace(), but we could find a common place if desired. During
development, I found that this file replicates the location-tracking
logic in the backend, but doesn't seem to make use of it. I also would
have had to replicate the backend's datatype for YYLTYPE. Fixing that
might be worthwhile some day, but to get this working, I just ripped
out the extra location tracking.

I no longer use state variables to track scanner state, and in fact I
removed the existing "state_before" variable in ECPG. Instead, I used
the Flex builtins yy_push_state(), yy_pop_state(), and yy_top_state().
These have been a feature for a long time, it seems, so I think we're
okay as far as portability. I think it's cleaner this way, and
possibly faster. I also used this to reunite the xcc and xcsql states.
This whole part could be split out into a separate refactoring patch
to be applied first, if desired.

-- 
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


v10-handle-uescapes-in-parser.patch
Description: Binary data

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Tue, Dec 3, 2019 at 4:25 PM Amit Kapila  wrote:

> Few other things, I would like you to consider.
> 1.  I think disable_parallel_leader_participation related code can be
> extracted into a separate patch as it is mainly a debug/test aid.  You can
> also fix the problem reported by Mahendra in that context.
>
> 2. I think if we cam somehow disallow very small indexes to use parallel
> workers, then it will be better.   Can we use  min_parallel_index_scan_size
> to decide whether a particular index can participate in a parallel vacuum?
>

Forgot one minor point.  Please run pgindent on all the patches.

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

Re: fe-utils - share query cancellation code

[Michael Paquier]

On Mon, Dec 02, 2019 at 11:54:02AM +0900, Michael Paquier wrote:
> Committed the patch after splitting things into two commits and after
> testing things from Linux and from a Windows console: the actual
> refactoring and the pgbench changes.

I have found that we have a useless declaration of CancelRequested in
common.h, which is already part of cancel.h.  On top of that I think
that we need to rework a bit the header inclusions of bin/scripts/, as
per the attached.  A small set of issues, still these are issues.
Sorry for having missed these.
--
Michael
diff --git a/src/bin/scripts/clusterdb.c b/src/bin/scripts/clusterdb.c
index 3aee5f2834..936dd62052 100644
--- a/src/bin/scripts/clusterdb.c
+++ b/src/bin/scripts/clusterdb.c
@@ -12,6 +12,7 @@
 #include "postgres_fe.h"
 #include "common.h"
 #include "common/logging.h"
+#include "fe_utils/cancel.h"
 #include "fe_utils/simple_list.h"
 #include "fe_utils/string_utils.h"
 
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index d2a7547441..680bbb133a 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -19,6 +19,7 @@
 
 #include "common.h"
 #include "common/logging.h"
+#include "fe_utils/cancel.h"
 #include "fe_utils/connect.h"
 #include "fe_utils/string_utils.h"
 
diff --git a/src/bin/scripts/common.h b/src/bin/scripts/common.h
index db2f85b472..33b952ba63 100644
--- a/src/bin/scripts/common.h
+++ b/src/bin/scripts/common.h
@@ -10,7 +10,6 @@
 #define COMMON_H
 
 #include "common/username.h"
-#include "fe_utils/cancel.h"
 #include "getopt_long.h"		/* pgrminclude ignore */
 #include "libpq-fe.h"
 #include "pqexpbuffer.h"		/* pgrminclude ignore */
@@ -22,8 +21,6 @@ enum trivalue
 	TRI_YES
 };
 
-extern bool CancelRequested;
-
 typedef void (*help_handler) (const char *progname);
 
 extern void handle_help_version_opts(int argc, char *argv[],
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index bedd95cf9d..65abe5984e 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -14,6 +14,7 @@
 #include "catalog/pg_class_d.h"
 #include "common.h"
 #include "common/logging.h"
+#include "fe_utils/cancel.h"
 #include "fe_utils/connect.h"
 #include "fe_utils/simple_list.h"
 #include "fe_utils/string_utils.h"
diff --git a/src/bin/scripts/scripts_parallel.c b/src/bin/scripts/scripts_parallel.c
index 97435160e9..a732f07dd8 100644
--- a/src/bin/scripts/scripts_parallel.c
+++ b/src/bin/scripts/scripts_parallel.c
@@ -24,6 +24,7 @@
 
 #include "common.h"
 #include "common/logging.h"
+#include "fe_utils/cancel.h"
 #include "scripts_parallel.h"
 
 static void init_slot(ParallelSlot *slot, PGconn *conn);
diff --git a/src/bin/scripts/vacuumdb.c b/src/bin/scripts/vacuumdb.c
index 83a94dc632..e1623e76e3 100644
--- a/src/bin/scripts/vacuumdb.c
+++ b/src/bin/scripts/vacuumdb.c
@@ -16,6 +16,7 @@
 
 #include "common.h"
 #include "common/logging.h"
+#include "fe_utils/cancel.h"
 #include "fe_utils/connect.h"
 #include "fe_utils/simple_list.h"
 #include "fe_utils/string_utils.h"


signature.asc
Description: PGP signature

Re: Yet another vectorized engine

[Konstantin Knizhnik]

On 02.12.2019 4:15, Hubert Zhang wrote:


The prototype extension is at https://github.com/zhangh43/vectorize_engine


I am very sorry, that I have no followed this link.
Few questions concerning your design decisions:

1. Will it be more efficient to use native arrays in vtype instead of 
array of Datum? I think it will allow compiler to generate more 
efficient code for operations with float4 and int32 types.

It is possible to use union to keep fixed size of vtype.
2. Why VectorTupleSlot contains array (batch) of heap tuples rather than 
vectors (array of vtype)?
3. Why you have to implement your own plan_tree_mutator and not using 
expression_tree_mutator?
4. As far as I understand you now always try to replace SeqScan with 
your custom vectorized scan. But it makes sense only if there are quals 
for this scan or aggregation is performed.

In other cases batch+unbatch just adds extra overhead, doesn't it?
5. Throwing and catching exception for queries which can not be 
vectorized seems to be not the safest and most efficient way of handling 
such cases.
May be it is better to return error code in plan_tree_mutator and 
propagate this error upstairs?
6. Have you experimented with different batch size? I have done similar 
experiments in VOPS and find out that tile size larger than 128 are not 
providing noticable increase of performance.
You are currently using batch size 1024 which is significantly larger 
than typical amount of tuples on one page.
7. How vectorized scan can be combined with parallel execution (it is 
already supported in9.6, isn't it?)


--

Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [PATCH] Implement INSERT SET syntax

[Gareth Palmer]

On Sun, Dec 1, 2019 at 4:32 PM Michael Paquier  wrote:
>
> On Fri, Nov 22, 2019 at 12:24:15PM +1300, Gareth Palmer wrote:
> > Attached is an updated patch with for_locking_clause added, test-cases
> > re-use existing tables and the comments and documentation have been
> > expanded.
>
> Per the automatic patch tester, documentation included in the patch
> does not build.  Could you please fix that?  I have moved the patch to
> next CF, waiting on author.

Attached is a fixed version.

> --
> Michael
From c7c32435f0c0a1948e5c3ebd7d66f0bc415ee54e Mon Sep 17 00:00:00 2001
From: Gareth Palmer 
Date: Mon, 2 Dec 2019 10:59:40 +
Subject: [PATCH] Implement INSERT SET syntax

Allow the target column and values of an INSERT statement to be specified
using a SET clause in the same manner as that of an UPDATE statement.

The advantage of using the INSERT SET style is that the columns and values
are kept together, which can make changing or removing a column or value
from a large list easier.

A simple example that uses SET instead of a VALUES() clause:

INSERT INTO t SET c1 = 'foo', c2 = 'bar', c3 = 'baz';

Values can also be sourced from other tables similar to the INSERT INTO
SELECT FROM syntax:

INSERT INTO t SET c1 = x.c1, c2 = x.c2 FROM x WHERE x.c2 > 10 LIMIT 10;

INSERT SET is not part of any SQL standard, however this syntax is also
implemented by MySQL. Their implementation does not support specifying a
FROM clause.
---
 doc/src/sgml/ref/insert.sgml  | 59 ++-
 src/backend/parser/gram.y | 58 +-
 src/backend/parser/parse_expr.c   | 10 +++-
 src/test/regress/expected/insert.out  | 26 +---
 src/test/regress/expected/insert_conflict.out |  2 +
 src/test/regress/expected/with.out| 20 +++
 src/test/regress/sql/insert.sql   |  2 +
 src/test/regress/sql/insert_conflict.sql  |  3 +
 src/test/regress/sql/with.sql |  9 +++
 9 files changed, 177 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/ref/insert.sgml b/doc/src/sgml/ref/insert.sgml
index e829c61642..b2f7c06f53 100644
--- a/doc/src/sgml/ref/insert.sgml
+++ b/doc/src/sgml/ref/insert.sgml
@@ -28,6 +28,19 @@ INSERT INTO table_name [ AS conflict_target ] conflict_action ]
 [ RETURNING * | output_expression [ [ AS ] output_name ] [, ...] ]
 
+
+[ WITH [ RECURSIVE ] with_query [, ...] ]
+INSERT INTO table_name [ AS alias ]
+[ OVERRIDING { SYSTEM | USER} VALUE ]
+SET { column_name = { expression | DEFAULT } |
+  ( column_name [, ...] ) = [ ROW ] ( { expression | DEFAULT } [, ...] ) |
+  ( column_name [, ...] ) = ( sub-SELECT )
+} [, ...]
+[ FROM from_clause ]
+[ ON CONFLICT [ conflict_target ] conflict_action ]
+[ RETURNING * | output_expression [ [ AS ] output_name ] [, ...] ]
+
+
 where conflict_target can be one of:
 
 ( { index_column_name | ( index_expression ) } [ COLLATE collation ] [ opclass ] [, ...] ) [ WHERE index_predicate ]
@@ -254,6 +267,18 @@ INSERT INTO table_name [ AS 
  
 
+ 
+  from_clause
+  
+   
+A list of table expressions, allowing columns from other tables
+to be used as values in the expression.
+Refer to the  statement for a
+description of the syntax.
+   
+  
+ 
+
  
   DEFAULT
   
@@ -631,6 +656,15 @@ INSERT INTO films (code, title, did, date_prod, kind) VALUES
 
   
 
+  
+Insert a row using SET syntax:
+
+
+INSERT INTO films SET code = 'MH832', title = 'Blade Runner',
+did = 201, date_prod = DEFAULT, kind = 'SciFi'; 
+
+  
+
   
This example inserts some rows into table
films from a table tmp_films
@@ -677,6 +711,16 @@ WITH upd AS (
 INSERT INTO employees_log SELECT *, current_timestamp FROM upd;
 
   
+  
+   Insert multiple rows into employees_log containing
+the hours worked by each employee from time_sheets.
+
+INSERT INTO employees_log SET id = time_sheets.employee,
+total_hours = sum(time_sheets.hours) FROM time_sheets
+WHERE time_sheets.date  '2019-11-15' GROUP BY time_sheets.employee;
+
+  
+
   
Insert or update new distributors as appropriate.  Assumes a unique
index has been defined that constrains values appearing in the
@@ -733,6 +777,18 @@ INSERT INTO distributors (did, dname) VALUES (9, 'Antwerp Design')
 INSERT INTO distributors (did, dname) VALUES (10, 'Conrad International')
 ON CONFLICT (did) WHERE is_active DO NOTHING;
 
+  
+   Insert a new film into watched_films or increment the
+   number of times seen. Returns the new seen count, example assumes a
+   unique index has been defined that constrains the values appearing in
+   the title and year columns and 
+   that seen_count defaults to 1.
+
+INSERT INTO watched_films SET title = 'Akira', year = 1988
+   ON CONFLICT (title, year) DO UPDATE SET seen_count = watched_films.seen_count + 1
+   RETURNING watched_films.seen_count;
+
+  
  
 
  
@@ -743,7 +799,8 

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

[Alexey Kondratov]

On 01.12.2019 5:57, Michael Paquier wrote:

On Thu, Sep 26, 2019 at 03:08:22PM +0300, Alexey Kondratov wrote:

As Alvaro correctly pointed in the nearby thread [1], we've got an
interference regarding -R command line argument. I agree that it's a good
idea to reserve -R for recovery configuration write to be consistent with
pg_basebackup, so I've updated my patch to use another letters:

The patch has rotten and does not apply anymore.  Could you please
send a rebased version?  I have moved the patch to next CF, waiting on
author for now.


Rebased and updated patch is attached.

There was a problem with testing new restore_command options altogether 
with recent ensureCleanShutdown. My test simply moves all WAL from 
pg_wal and generates restore_command for a new options testing, but this 
prevents startup recovery required by ensureCleanShutdown. To test both 
options in the same we have to leave some recent WAL segments in the 
pg_wal and be sure that they are enough for startup recovery, but not 
enough for successful pg_rewind run. I have manually figured out that 
required amount of inserted records (and generated WAL) to achieve this. 
However, I think that this approach is not good for test, since tests 
may be modified in the future (amount of writes to DB changed) or even 
volume of WAL written by Postgres will change. It will lead to falsely 
always failed or passed tests.


Moreover, testing both ensureCleanShutdown and new options in the same 
time doesn't hit new code paths, so I decided to test new options with 
--no-ensure-shutdown for simplicity and stability of tests.



Regards

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company

>From a05c3343e0bd6fe339c944f6b0cde64ceb46a0b3 Mon Sep 17 00:00:00 2001
From: Alexey Kondratov 
Date: Tue, 19 Feb 2019 19:14:53 +0300
Subject: [PATCH v11] pg_rewind: options to use restore_command from command
 line or cluster config

Previously, when pg_rewind could not find required WAL files in the
target data directory the rewind process would fail. One had to
manually figure out which of required WAL files have already moved to
the archival storage and copy them back.

This patch adds possibility to specify restore_command via command
line option or use one specified inside postgresql.conf. Specified
restore_command will be used for automatic retrieval of missing WAL
files from archival storage.
---
 doc/src/sgml/ref/pg_rewind.sgml   |  49 +++-
 src/bin/pg_rewind/parsexlog.c | 164 +-
 src/bin/pg_rewind/pg_rewind.c | 118 +++---
 src/bin/pg_rewind/pg_rewind.h |   6 +-
 src/bin/pg_rewind/t/001_basic.pl  |   4 +-
 src/bin/pg_rewind/t/002_databases.pl  |   4 +-
 src/bin/pg_rewind/t/003_extrafiles.pl |   4 +-
 src/bin/pg_rewind/t/RewindTest.pm | 105 -
 8 files changed, 416 insertions(+), 38 deletions(-)

diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml
index 42d29edd4e..b601a5c7e4 100644
--- a/doc/src/sgml/ref/pg_rewind.sgml
+++ b/doc/src/sgml/ref/pg_rewind.sgml
@@ -66,11 +66,12 @@ PostgreSQL documentation
can be found either on the target timeline, the source timeline, or their common
ancestor. In the typical failover scenario where the target cluster was
shut down soon after the divergence, this is not a problem, but if the
-   target cluster ran for a long time after the divergence, the old WAL
-   files might no longer be present. In that case, they can be manually
-   copied from the WAL archive to the pg_wal directory, or
-   fetched on startup by configuring  or
-   .  The use of
+   target cluster ran for a long time after the divergence, its old WAL
+   files might no longer be present. In this case, you can manually copy them
+   from the WAL archive to the pg_wal directory, or run
+   pg_rewind with the -c or
+   -C option to automatically retrieve them from the WAL
+   archive. The use of
pg_rewind is not limited to failover, e.g.  a standby
server can be promoted, run some write transactions, and then rewinded
to become a standby again.
@@ -232,6 +233,39 @@ PostgreSQL documentation
   
  
 
+ 
+  -c
+  --restore-target-wal
+  
+   
+Use the restore_command defined in
+postgresql.conf to retrieve WAL files from
+the WAL archive if these files are no longer available in the
+pg_wal directory of the target cluster.
+   
+   
+This option cannot be used together with --target-restore-command.
+   
+  
+ 
+
+ 
+  -C restore_command
+  --target-restore-command=restore_command
+  
+   
+Specifies the restore_command to use for retrieving
+WAL files from the WAL archive if these files are no longer available
+in the pg_wal directory of the target cluster.
+   
+   
+If restore_command is already set in
+

Re: Update minimum SSL version

[Magnus Hagander]

On Tue, Dec 3, 2019 at 4:53 AM Michael Paquier  wrote:

> On Mon, Dec 02, 2019 at 12:51:26PM -0500, Tom Lane wrote:
> > Yah.  Although, looking at the code in be-secure-openssl.c,
> > it doesn't look that hard to do in an extensible way.
> > Something like (untested)
>
> While we are on the topic...  Here is another wild idea.  We discussed
> not so long ago about removing support for OpenSSL 0.9.8 from the
> tree.  What if we removed support for 1.0.0 and 0.9.8 for 13~.  This
> would solve a couple of compatibility headaches, and we have TLSv1.2
> support automatically for all the versions supported.  Note that 1.0.0
> has been retired by upstream in February 2014.
>

Is 1.0.1 considered a separate major from 1.0.0, in this reasoning? Because
while retiring 1.0.0 should probably not be that terrible, 1.0.1 is still
in very widespread use on most long term supported distributions.

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

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Tue, Dec 3, 2019 at 12:55 PM tushar 
wrote:

> On 11/27/19 11:13 PM, Masahiko Sawada wrote:
> > Thank you for reviewing this patch. All changes you made looks good to
> me.
> >
> > I thought I already have posted all v34 patches but didn't, sorry. So
> > I've attached v35 patch set that incorporated your changes and it
> > includes Dilip's patch for gist index (0001). These patches can be
> > applied on top of the current HEAD and make check should pass.
> > Regards,
> While doing testing of this feature against v35- patches ( minus 004) on
> Master ,
>

Thanks for doing the testing of these patches.


> getting crash when user connect to server using single mode and try to
> perform vacuum (parallel 1 ) o/p
>
> tushar@localhost bin]$ ./postgres --single -D data/  postgres
> 2019-12-03 12:49:26.967 +0530 [70300] LOG:  database system was
> interrupted; last known up at 2019-12-03 12:48:51 +0530
> 2019-12-03 12:49:26.987 +0530 [70300] LOG:  database system was not
> properly shut down; automatic recovery in progress
> 2019-12-03 12:49:26.990 +0530 [70300] LOG:  invalid record length at
> 0/29F1638: wanted 24, got 0
> 2019-12-03 12:49:26.990 +0530 [70300] LOG:  redo is not required
>
> PostgreSQL stand-alone backend 13devel
> backend>
> backend> vacuum full;
> backend> vacuum (parallel 1);
>

The parallel vacuum shouldn't be allowed via standalone backends as we
can't create DSM segments in that mode and similar is true for the parallel
query.  It should internally proceed with a serial vacuum.  I'll fix it in
the next version I am planning to post.  BTW, it seems that the same
problem will be there for parallel create index.

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

Re: Collation versioning

[Julien Rouhaud]

On Mon, Dec 2, 2019 at 2:00 PM Robert Haas  wrote:
>
> ALTER INDEX idx_name DEPENDS ON COLLATION blah VERSION blah;
> -- I care about collations and I know which one and which version.
>
> ALTER INDEX idx_name DEPENDS ON SOME COLLATION;
> -- I care about collations but I don't know which one.

This seems a little bit ambiguous.  I wouldn't expect this command to
trash all recorded versions given how it's spelled.

> ALTER INDEX idx_name DEPENDS ON NO COLLATION;
> -- I don't care about collations at all.
> -- Not sure if we need this.

This should be an alias for "trust me all collation are ok"?  It's
certainly useful for collation library upgrade that don't break
indexes, but I'd prefer to spell it something like "CURRENT VERSION".
I'll also work on that, but preferably in a later patch as there'll be
additional need (process all indexes with a given collation or
collation version for instance).


While looking at the list of keywords again, I think that "ANY" is a
better candidate:

ALTER INDEX idx_name DEPENDS ON [ ANY COLLATION | COLLATION blah ] [
UNKNOWN VERSION | VERSION blah ]
or
ALTER INDEX idx_name ALTER [ ANY COLLATION | COLLATION blah ] DEPENDS
ON  [ UNKNOWN VERSION  | VERSION blah ]

I like the 2nd one as it's more obvious that the command will only
modify existing dependencies.