RE: walsender performance regression due to logical decoding on standby changes

[Zhijie Hou (Fujitsu)]

On Friday, May 19, 2023 11:08 AM Kyotaro Horiguchi  
wrote:
> At Thu, 18 May 2023 20:11:11 +0530, Bharath Rupireddy
>  wrote in
> > > > + ConditionVariableInit(&WalSndCtl->physicalWALSndCV);
> > > > + ConditionVariableInit(&WalSndCtl->logicalWALSndCV);
> > >
> > > It's not obvious to me that it's worth having two CVs, because it's more
> > > expensive to find no waiters in two CVs than to find no waiters in one CV.
> >
> > I disagree. In the tight per-WAL record recovery loop, WalSndWakeup
> > wakes up logical walsenders for every WAL record, but it wakes up
> > physical walsenders only if the applied WAL record causes a TLI
> > switch. Therefore, the extra cost of spinlock acquire-release for per
> > WAL record applies only for logical walsenders. On the other hand, if
> > we were to use a single CV, we would be unnecessarily waking up (if at
> > all they are sleeping) physical walsenders for every WAL record -
> > which is costly IMO.
> 
> As I was reading this, I start thinking that one reason for the
> regression could be to exccessive frequency of wakeups during logical
> replication. In physical replication, we make sure to avoid exccessive
> wakeups when the stream is tightly packed.  I'm just wondering why
> logical replication doesn't (or can't) do the same thing, IMHO.

I thought(from the e101dfa's commit message) physical walsenders can't send
data until it's been flushed, so it only gets wakeup at the time of flush or TLI
switch. For logical walsender, it can start to decode changes after the change
is applied, and to avoid the case if the walsender is asleep, and there's work
to be done, it wakeup logical walsender when applying each record.

Or maybe you mean to wakeup(ConditionVariableBroadcast) walsender after
applying some wal records like[1], But it seems it may delay the wakeup of
walsender a bit(the walsender may be asleep before reaching the threshold).




[1] 
--- a/src/backend/access/transam/xlogrecovery.c
+++ b/src/backend/access/transam/xlogrecovery.c
@@ -1833,6 +1833,7 @@ ApplyWalRecord(XLogReaderState *xlogreader, XLogRecord 
*record, TimeLineID *repl
 {
ErrorContextCallback errcallback;
boolswitchedTLI = false;
+   static int  nreplay = 0;
 
/* Setup error traceback support for ereport() */
errcallback.callback = rm_redo_error_callback;
@@ -1957,8 +1958,12 @@ ApplyWalRecord(XLogReaderState *xlogreader, XLogRecord 
*record, TimeLineID *repl
 *be created otherwise)
 * --
 */
-   if (AllowCascadeReplication())
+   if (AllowCascadeReplication() &&
+   nreplay++ == 100)
+   {
WalSndWakeup(switchedTLI, true);
+   nreplay = 0;
+   }

Best Regards,
Hou zj

PG 16 draft release notes ready

[Hans Buschmann]

I observed a missing end bracket in E 1.3.11:


Require Windows 10 or newer versions (Michael Paquier, Juan José Santamaría 
Flecha


Hans Buschmann

Re: ERROR: wrong varnullingrels (b 5 7) (expected (b)) for Var 3/3

[Richard Guo]

On Fri, May 19, 2023 at 1:57 PM Michael Paquier  wrote:

> Thanks for the test case, issue reproduced here on HEAD and not v15.
> This causes an assertion failure here:
> #4  0x55a6f8faa776 in ExceptionalCondition
> (conditionName=0x55a6f915ac60 "bms_equal(rel->relids,
> root->all_query_rels)", fileName=0x55a6f915ac3d "allpaths.c",
> lineNumber=234) at assert.c:66
> #5  0x55a6f8c55b6d in make_one_rel (root=0x55a6fa814ea8,
> joinlist=0x55a6fa83f758) at allpaths.c:234
> #6  0x55a6f8c95c45 in query_planner (root=0x55a6fa814ea8,
> qp_callback=0x55a6f8c9c373 , qp_extra=0x7ffc98138570)
> at planmain.c:278
> #7  0x55a6f8c9860a in grouping_planner (root=0x55a6fa814ea8,
> tuple_fraction=0) at planner.c:1493
>
> I am adding an open item.


Thanks for testing.  I looked into it and figured out that it is the
same issue discussed at the end of this thread:
https://www.postgresql.org/message-id/CAMbWs4-EU9uBGSP7G-iTwLBhRQ%3DrnZKvFDhD%2Bn%2BxhajokyPCKg%40mail.gmail.com

Thanks
Richard

Re: Introduce WAIT_EVENT_EXTENSION and WAIT_EVENT_BUFFER_PIN

[Drouvot, Bertrand]

Hi,

On 5/19/23 12:36 AM, Michael Paquier wrote:

On Thu, May 18, 2023 at 12:28:20PM -0400, Robert Haas wrote:

I mean, I agree that it would probably be hard to measure any real
performance difference. But I'm not sure that's a good reason to add
cycles to a path where we don't really need to.


Honestly, I am not sure that it's worth worrying about performance
here,


Same feeling here and as those new functions will be used "only" from
pg_stat_get_activity() / pg_stat_get_backend_wait_event().


or perhaps you know of some external stuff that could set the
extension class type in a code path hot enough that it would matter.


And that would matter, only when making use of pg_stat_get_activity()
/ pg_stat_get_backend_wait_event() at the time the "extension is waiting"
on this wait event.

While at it, I think that making use of an enum might also be an open door
(need to think more about it) to allow extensions to set their own wait event.
Something like RequestNamedLWLockTranche()/GetNamedLWLockTranche() are doing.

Currently we have "only" the "extension" wait event which is not that useful 
when
there is multiples extensions installed in a database.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: PG 16 draft release notes ready

[Drouvot, Bertrand]

Hi,

On 5/18/23 10:49 PM, Bruce Momjian wrote:

I have completed the first draft of the PG 16 release notes.  You can
see the output here:

https://momjian.us/pgsql_docs/release-16.html

I will adjust it to the feedback I receive;  that URL will quickly show
all updates.



Thanks!

"
This adds the function pg_log_standby_snapshot(). TEXT?:
"

My proposal:

This adds the function pg_log_standby_snapshot() to log details of the current 
snapshot
to WAL. If the primary is idle, the slot creation on a standby can take a while.
This function can be used on the primary to speed up the logical slot creation 
on
the standby.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

New COPY options: DELIMITER NONE and QUOTE NONE

[Joel Jacobson]

The thread "Should CSV parsing be stricter about mid-field quotes?" [1] forked
into a new topic, with two new ideas, hence this new thread.

1. COPY ... QUOTE NONE

In the [1] thread, Andrew Dunstan suggested a trick on how to deal with
unquoted but delimited files, such as TSV-files produced by Google Sheets:

> You can use CSV mode pretty reliably for TSV files.
> The trick is to use a quoting char that shouldn't appear,
> such as E'\x01' as well as setting the delimiter to E'\t'.
> Yes, it's far from obvious.

Would it be an improvement to allow specifying `QUOTE NONE` instead?

quotes.tsv:
id quote
1 "E = mc^2" -- Albert Einstein

COPY quotes FROM '/tmp/quotes.tsv' WITH CSV HEADER DELIMITER E'\t' QUOTE NONE;

SELECT * FROM quotes;
id | quote
+---
  1 | "E = mc^2" -- Albert Einstein
(1 row)

2. COPY ... DELIMITER NONE

This is meant to improve the use-case when wanting to import e.g. an
unstructured log file line-by-line into a single column.

The current trick I've been using is similar to the first one,
that is, to specify a non-existing delimiter. But that involves having to find
some non-existing byte, which is error-prone since future log files might
suddenly start to contain it. So I think it would be better to be to be explicit
about not wanting to delimit fields at all, treating the entire whole line as a 
column.

Example:

% cat /tmp/apache.log
192.168.1.1 - - [19/May/2023:09:54:17 -0700] "GET /index.html HTTP/1.1" 200 431 
"http://www.example.com/home.html"; "Mozilla/5.0 (Windows NT 10.0; Win64; x64) 
AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.3"
192.168.1.2 - - [19/May/2023:09:55:12 -0700] "POST /form.php HTTP/1.1" 200 512 
"http://www.example.com/form.html"; "Mozilla/5.0 (Windows NT 10.0; Win64; x64) 
AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.3"

CREATE TABLE unstructured_log (whole_line text);
COPY unstructured_log FROM '/tmp/apache.log' WITH CSV DELIMITER NONE QUOTE NONE;
SELECT * FROM unstructured_log;

   whole_line
-
192.168.1.1 - - [19/May/2023:09:54:17 -0700] "GET /index.html HTTP/1.1" 200 431 
"http://www.example.com/home.html"; "Mozilla/5.0 (Windows NT 10.0; Win64; x64) 
AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.3"
192.168.1.2 - - [19/May/2023:09:55:12 -0700] "POST /form.php HTTP/1.1" 200 512 
"http://www.example.com/form.html"; "Mozilla/5.0 (Windows NT 10.0; Win64; x64) 
AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.3"
(2 rows)

I hacked together a broken patch just to demonstrate the idea on syntax
and basic idea. The `COPY ... FROM` examples above works.
But it doesn't work at all for `COPY ... TO`, since it output \0 byte as
delimiter and quote in the output, which is of course not what we want.

Just wanted some feedback to see if there is any broader interest in this,
before proceeding and looking into how to implement it properly.

Is this something we want or are there just a few of us who have needed this in 
the past?

/Joel

[1] 
https://www.postgresql.org/message-id/31c81233-d707-0d2a-8111-a915f463459b%40dunslane.net

copy_delimiter_quote_none.patch
Description: Binary data

Re: very long record lines in expanded psql output

[Alvaro Herrera]

On 2022-Jul-25, Andrew Dunstan wrote:

> Committed. There were a couple of bits missing, which I filled in, and I
> changed the behaviour when the option is given without an argument, so
> that instead of resetting it the current value is shown, similarly to
> how most pset options work.

I was translating the new messages introduced by this commit, and
decided to unify them.  While doing so, I notice that the feature
misbehaves when you give it a string value that doesn't match any valid
value: it just resets the value to 0, which is entirely the wrong thing.
For other settings, we first verify that the given value is valid, and
only then we change the setting.

So I came up with the attached.  While at it, I noticed that we have
other uses of atoi() there, most notably pager_min_lines.  My first
impulse was to just to do the same as for xheader_width, namely to test
whether the atoid result is zero, and not change in that case.  But then
I noticed we're already using variables.c facilities, so I decided to do
likewise; this results in simpler code.  So we're now stricter, because
variables.c rejects trailing junk after the number.  (123asd was
previously parsed as 123, now it's an error.)


There remain atoi uses in this code, and aside from the atoi()-induced
behavior of making any non-number input set it to 0, it does stuff like

=# \pset border 65538asd
Border style is 2.

because border style is short int, so this value overflows it.  Same
happens with 'columns'.  However, this is all very old code and nobody
seems to have complained.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)
>From 5eb799984252cf231064d6d174e13d63880577df Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Fri, 19 May 2023 12:58:54 +0200
Subject: [PATCH] Tweak xheader_width input parsing

Don't throw away the previous value when an invalid value is proposed.
Also, change the error messages to not need separate translations.

While at it, change pager_min_lines to also avoid changing the setting
when an invalid value is given.
---
 src/bin/psql/command.c | 20 
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 607a57715a3..07c5f026b9b 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -4521,13 +4521,16 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 			popt->topt.expanded_header_width_type = PRINT_XHEADER_PAGE;
 		else
 		{
-			popt->topt.expanded_header_width_type = PRINT_XHEADER_EXACT_WIDTH;
-			popt->topt.expanded_header_exact_width = atoi(value);
-			if (popt->topt.expanded_header_exact_width == 0)
+			int		intval = atoi(value);
+
+			if (intval == 0)
 			{
 pg_log_error("\\pset: allowed xheader_width values are full (default), column, page, or a number specifying the exact width.");
 return false;
 			}
+
+			popt->topt.expanded_header_width_type = PRINT_XHEADER_EXACT_WIDTH;
+			popt->topt.expanded_header_exact_width = intval;
 		}
 	}
 
@@ -4660,8 +4663,9 @@ do_pset(const char *param, const char *value, printQueryOpt *popt, bool quiet)
 	/* set minimum lines for pager use */
 	else if (strcmp(param, "pager_min_lines") == 0)
 	{
-		if (value)
-			popt->topt.pager_min_lines = atoi(value);
+		if (value &&
+			!ParseVariableNum(value, "pager_min_lines", &popt->topt.pager_min_lines))
+			return false;
 	}
 
 	/* disable "(x rows)" footer */
@@ -4727,11 +4731,11 @@ printPsetInfo(const char *param, printQueryOpt *popt)
 	else if (strcmp(param, "xheader_width") == 0)
 	{
 		if (popt->topt.expanded_header_width_type == PRINT_XHEADER_FULL)
-			printf(_("Expanded header width is 'full'.\n"));
+			printf(_("Expanded header width is '%s'.\n"), "full");
 		else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_COLUMN)
-			printf(_("Expanded header width is 'column'.\n"));
+			printf(_("Expanded header width is '%s'.\n"), "column");
 		else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_PAGE)
-			printf(_("Expanded header width is 'page'.\n"));
+			printf(_("Expanded header width is '%s'.\n"), "page");
 		else if (popt->topt.expanded_header_width_type == PRINT_XHEADER_EXACT_WIDTH)
 			printf(_("Expanded header width is %d.\n"), popt->topt.expanded_header_exact_width);
 	}

base-commit: 0b8ace8d773257fffeaceda196ed94877c2b74df
-- 
2.39.2

Re: The documentation for READ COMMITTED may be incomplete or wrong

[Aleksander Alekseev]

Hi,

> The attached v2 mechanizes  that using two bool arrays.

I tested the patch on several combinations of operating systems
(LInux, MacOS) and architectures (x64, RISC-V) available to me at the
moment, with both Meson and Autotools. Also I made sure
eval-plan-qual.spec fails when the C code is untouched.

The patch passed all the checks I could come up with.

--
Best regards,
Aleksander Alekseev

How to connect with PostgreSQL Database with SSL using Certificates and Key from client Eclipse in Java

[sujay kadam]

I am trying to connect with PostgreSQL database from client with SSL
enabled on server 10.30.32.186 port 6432 using below java code -

I am using certificates ( [server-cert.pem, server-key.pem, ca.cert] and
[postgresql.crt, postgresql.pk8, root.crt] ).

Suggest me if there are any specific java understandable certificate and
key file format.


package com.ssl;

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;

public class DBConnect {

private final String url = "jdbc:postgresql://
10.30.32.186:6432/postgres?sslmode=require&sslcert=/root/.postgresql/postgresql.crt&sslkey=/root/.postgresql/postgresql.pk8&sslrootcert=/root/.postgresql/root.crt&sslpassword=postgress
";

private final String user = "postgres";
private final String password = "postgres123";

/**
 * Connect to the PostgreSQL database
 *
 * @return a Connection object
 */
public Connection connect() {
Connection conn = null;
try {
conn = DriverManager.getConnection(url, user, password);
System.out.println("Connected to the PostgreSQL server
successfully.");
} catch (SQLException e) {
System.out.println(e.getMessage());
}

return conn;
}

public static void main(String[] args) {

DBConnect db = new DBConnect();
db.connect();

}

}

Gives Error -

SSL error: -1



Code NO 2 -

package SSL_Enablement;

import java.sql.Connection;
import java.sql.DriverManager;
import java.sql.SQLException;
import java.util.Properties;

public class PostgresSSLConnection {
public static void main(String[] args) {
Connection conn = null;
try {
// Set SSL properties
Properties props = new Properties();
props.setProperty("user", "postgres");
props.setProperty("password", "postgres123");
props.setProperty("ssl", "true");
props.setProperty("https.protocols", "TLSv1.2");
props.setProperty("sslmode", "Verify-CA");
props.setProperty("sslcert",
"/root/.postgresql/server-cert.pem");
props.setProperty("sslkey", "/root/.postgresql/server-key.pem");
props.setProperty("sslrootcert", "/root/.postgresql/ca.cert");

// Initialize SSL context
Class.forName("org.postgresql.Driver");
String url = "jdbc:postgresql://10.30.32.186:6432/postgres";
conn = DriverManager.getConnection(url, props);
System.out.println("Connected DB using SSL");
// Use the connection...
// ...

} catch (SQLException e) {
e.printStackTrace();
} catch (ClassNotFoundException e) {
e.printStackTrace();
} finally {
try {
if (conn != null) {
conn.close();
}
} catch (SQLException e) {
e.printStackTrace();
}
}
}
}

Gives Error -

org.postgresql.util.PSQLException: Could not read SSL key file
/root/.postgresql/server-key.pem.
 at org.postgresql.ssl.LazyKeyManager.getPrivateKey(LazyKeyManager.java:284)
 at
sun.security.ssl.AbstractKeyManagerWrapper.getPrivateKey(SSLContextImpl.java:1552)
 at
sun.security.ssl.X509Authentication$X509PossessionGenerator.createClientPossession(X509Authentication.java:220)
 at
sun.security.ssl.X509Authentication$X509PossessionGenerator.createPossession(X509Authentication.java:175)
 at
sun.security.ssl.X509Authentication.createPossession(X509Authentication.java:88)
 at
sun.security.ssl.CertificateMessage$T13CertificateProducer.choosePossession(CertificateMessage.java:1080)
 at
sun.security.ssl.CertificateMessage$T13CertificateProducer.onProduceCertificate(CertificateMessage.java:1101)
 at
sun.security.ssl.CertificateMessage$T13CertificateProducer.produce(CertificateMessage.java:958)
 at sun.security.ssl.SSLHandshake.produce(SSLHandshake.java:421)
 at
sun.security.ssl.Finished$T13FinishedConsumer.onConsumeFinished(Finished.java:989)
 at sun.security.ssl.Finished$T13FinishedConsumer.consume(Finished.java:852)
 at sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:377)
 at sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:444)
 at sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:422)
 at sun.security.ssl.TransportContext.dispatch(TransportContext.java:182)
 at sun.security.ssl.SSLTransport.decode(SSLTransport.java:152)
 at sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1397)
 at
sun.security.ssl.SSLSocketImpl.readHandshakeRecord(SSLSocketImpl.java:1305)
 at sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:440)
 at org.postgresql.ssl.MakeSSL.convert(MakeSSL.java:41)
 at
org.postgresql.core.v3.ConnectionFactoryImpl.enableSSL(ConnectionFactoryImpl.java:584)
 at
org.postgresql.core.v3.ConnectionFactoryImpl.tryConnect(ConnectionFactoryImpl.java:168)

Re: How to connect with PostgreSQL Database with SSL using Certificates and Key from client Eclipse in Java

[Aleksander Alekseev]

Hi Sujay,

> I am trying to connect with PostgreSQL database from client with SSL enabled 
> on server 10.30.32.186 port 6432 using below java code -

This mailing list is dedicated to the PostgreSQL Core development. I
don't think you will find many people interested in your question
and/or familiar with Java.

I think you should address the question to pgsql-general@ mailing list
or StackOverflow.

(If you believe there is a bug in the DBMS core please provide simpler
steps to reproduce, ideally with pgsql utility and maybe bash.)

-- 
Best regards,
Aleksander Alekseev

Re: How to connect with PostgreSQL Database with SSL using Certificates and Key from client Eclipse in Java

[sujay kadam]

Thank you for your response.

On Fri, 19 May 2023 at 5:11 PM, Aleksander Alekseev <
aleksan...@timescale.com> wrote:

> Hi Sujay,
>
> > I am trying to connect with PostgreSQL database from client with SSL
> enabled on server 10.30.32.186 port 6432 using below java code -
>
> This mailing list is dedicated to the PostgreSQL Core development. I
> don't think you will find many people interested in your question
> and/or familiar with Java.
>
> I think you should address the question to pgsql-general@ mailing list
> or StackOverflow.
>
> (If you believe there is a bug in the DBMS core please provide simpler
> steps to reproduce, ideally with pgsql utility and maybe bash.)
>
> --
> Best regards,
> Aleksander Alekseev
>

Re: PG 16 draft release notes ready

[br...@momjian.us]

On Fri, May 19, 2023 at 07:42:12AM +, Hans Buschmann wrote:
> I observed a missing end bracket in E 1.3.11:
> 
> 
> Require Windows 10 or newer versions (Michael Paquier, Juan José Santamaría
> Flecha

Thanks, fixed.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Naming of gss_accept_deleg

[Bruce Momjian]

Why is the new PG 16 GUC called "gss_accept_deleg" and not
"gss_accept_delegation"?  The abbreviation here seems atypical.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: PG 16 draft release notes ready

[Bruce Momjian]

On Fri, May 19, 2023 at 09:49:18AM +0200, Drouvot, Bertrand wrote:
> Thanks!
> 
> "
> This adds the function pg_log_standby_snapshot(). TEXT?:
> "
> 
> My proposal:
> 
> This adds the function pg_log_standby_snapshot() to log details of the 
> current snapshot
> to WAL. If the primary is idle, the slot creation on a standby can take a 
> while.
> This function can be used on the primary to speed up the logical slot 
> creation on
> the standby.

Yes, I got this concept from the commit message, but I am unclear on
what is actually happening so I can clearly explain it.  Slot creation
on the standby needs a snapshot, and that is only created when there is
activity, or happens periodically, and this forces it to happen, or
something?  And what snapshot is this?  The current session's?

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: The documentation for READ COMMITTED may be incomplete or wrong

[Amit Langote]

Thanks for the patch.

On Fri, May 19, 2023 at 8:57 AM Tom Lane  wrote:
> I wrote:
> > Debian Code Search doesn't know of any outside code touching
> > relsubs_done, so I think we are safe in dropping that code in
> > ExecScanReScan.  It seems quite pointless anyway considering
> > that up to now, EvalPlanQualBegin has always zeroed the whole
> > array.
>
> Oh, belay that.  What I'd forgotten is that it's possible that
> the target relation is on the inside of a nestloop, meaning that
> we might need to fetch the EPQ substitute tuple more than once.
> So there are three possible states: blocked (never return a
> tuple), ready to return a tuple, and done returning a tuple
> for this scan.  ExecScanReScan needs to reset "done" to "ready",
> but not touch the "blocked" state.  The attached v2 mechanizes
> that using two bool arrays.

Aha, that's clever.  So ExecScanReScan() would only reset the
relsubs_done[] entry for the currently active ("unblocked") target
relation, because that would be the only one "unblocked" during a
given EvalPlanQual() invocation.

+* Initialize per-relation EPQ tuple states.  Result relations, if any,
+* get marked as blocked; others as not-fetched.

Would it be helpful to clarify that "blocked" means blocked for a
given EvalPlanQual() cycle?

+   /*
+* relsubs_blocked[scanrelid - 1] is true if there is no EPQ tuple for
+* this target relation.
+*/
+   bool   *relsubs_blocked;

Similarly, maybe say "no EPQ tuple for this target relation in a given
EvalPlanQual() invocation" here?

BTW, I didn't quite understand why EPQ involving resultRelations must
behave in this new way but not the EPQ during LockRows?

> What I'm thinking about doing to back-patch this is to replace
> one of the pointer fields in EPQState with a pointer to a
> subsidiary palloc'd structure, where we can put the new fields
> along with the cannibalized old one.  We've done something
> similar before, and it seems a lot safer than having basically
> different logic in v16 than earlier branches.

+1.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Re: Naming of gss_accept_deleg

[Stephen Frost]

Greetings,

* Bruce Momjian (br...@momjian.us) wrote:
> Why is the new PG 16 GUC called "gss_accept_deleg" and not
> "gss_accept_delegation"?  The abbreviation here seems atypical.

At the time it felt natural to me but I don't feel strongly about it,
happy to change it if folks would prefer it spelled out.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Naming of gss_accept_deleg

[Bruce Momjian]

On Fri, May 19, 2023 at 09:07:26AM -0400, Stephen Frost wrote:
> Greetings,
> 
> * Bruce Momjian (br...@momjian.us) wrote:
> > Why is the new PG 16 GUC called "gss_accept_deleg" and not
> > "gss_accept_delegation"?  The abbreviation here seems atypical.
> 
> At the time it felt natural to me but I don't feel strongly about it,
> happy to change it if folks would prefer it spelled out.

Yes, please do spell it out, thanks.  The fact "deleg" looks similar to
"debug" also doesn't help.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Naming of gss_accept_deleg

[Abhijit Menon-Sen]

At 2023-05-19 09:16:09 -0400, br...@momjian.us wrote:
>
> On Fri, May 19, 2023 at 09:07:26AM -0400, Stephen Frost wrote:
> > 
> > > Why is the new PG 16 GUC called "gss_accept_deleg" and not
> > > "gss_accept_delegation"?  The abbreviation here seems atypical.
> > 
> > At the time it felt natural to me but I don't feel strongly about it,
> > happy to change it if folks would prefer it spelled out.
> 
> Yes, please do spell it out, thanks.  The fact "deleg" looks similar to
> "debug" also doesn't help.

Note that GSS-API itself calls it the "DELEG" flag:

if (conn->gcred != GSS_C_NO_CREDENTIAL)
gss_flags |= GSS_C_DELEG_FLAG;

I would also prefer a GUC named gss_accept_delegation, but the current
name matches the libpq gssdeleg connection parameter and the PGSSDELEG
environment variable. Maybe there's something to be said for keeping
those three things alike?

-- Abhijit

Re: Naming of gss_accept_deleg

[Tom Lane]

Abhijit Menon-Sen  writes:
> I would also prefer a GUC named gss_accept_delegation, but the current
> name matches the libpq gssdeleg connection parameter and the PGSSDELEG
> environment variable. Maybe there's something to be said for keeping
> those three things alike?

+1 for spelling it out in all user-visible names.  I do not think
that that GSS-API C symbol is a good precedent to follow.

regards, tom lane

Re: Naming of gss_accept_deleg

[Bruce Momjian]

On Fri, May 19, 2023 at 09:42:00AM -0400, Tom Lane wrote:
> Abhijit Menon-Sen  writes:
> > I would also prefer a GUC named gss_accept_delegation, but the current
> > name matches the libpq gssdeleg connection parameter and the PGSSDELEG
> > environment variable. Maybe there's something to be said for keeping
> > those three things alike?
> 
> +1 for spelling it out in all user-visible names.  I do not think
> that that GSS-API C symbol is a good precedent to follow.

Once nice bonus of the release notes is that it allows us to see the new
API in one place to check for consistency.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: PG 16 draft release notes ready

[Nathan Bossart]

On Thu, May 18, 2023 at 04:49:47PM -0400, Bruce Momjian wrote:
> I have completed the first draft of the PG 16 release notes.  You can
> see the output here:
> 
>   https://momjian.us/pgsql_docs/release-16.html
> 
> I will adjust it to the feedback I receive;  that URL will quickly show
> all updates.

Thanks!

> Allow GRANT to give vacuum and analyze permission to users beyond the
> table owner or superusers (Nathan Bossart)

This one was effectively reverted in favor of the MAINTAIN privilege.

> Create a predefined role with permission to perform maintenance
> operations (Nathan Bossart)

IMO this should also mention the grantable MAINTAIN privilege.
Alternatively, the item above about granting vacuum/analyze privileges
could be adjusted.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: PostgreSQL 16 Beta 1 release announcement draft

[Nathan Bossart]

On Fri, May 19, 2023 at 12:17:50AM -0400, Jonathan S. Katz wrote:
> Attached is a draft of the release announcement for PostgreSQL 16 Beta 1.
> The goal of this announcement is to get people excited about testing the
> beta and highlight many of the new features.

Thanks!

> PostgreSQL 16 continues to give users to the ability grant privileged access 
> to
> features without requiring superuser with new
> [predefined 
> roles](https://www.postgresql.org/docs/devel/predefined-roles.html).
> These include `pg_maintain`, which enables execution of operations such as
> `VACUUM`, `ANALYZE`, `REINDEX`, and others, and `pg_createsubscription`, which
> allows users to create a logical replication subscription. Additionally,
> starting with release, logical replication subscribers execute transactions 
> on a
> table as the table owner, not the superuser.

[pg_use_]reserved_connections might also deserve a mention here.  AFAICT
it's the only new predefined role that isn't mentioned in the announcement.
I'm okay with leaving it out if folks don't think it should make the cut.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Naming of gss_accept_deleg

[Nathan Bossart]

On Fri, May 19, 2023 at 09:42:00AM -0400, Tom Lane wrote:
> Abhijit Menon-Sen  writes:
>> I would also prefer a GUC named gss_accept_delegation, but the current
>> name matches the libpq gssdeleg connection parameter and the PGSSDELEG
>> environment variable. Maybe there's something to be said for keeping
>> those three things alike?
> 
> +1 for spelling it out in all user-visible names.  I do not think
> that that GSS-API C symbol is a good precedent to follow.

+1

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Conflict between regression tests namespace & transactions due to recent changes

[Tom Lane]

Marina Polyakova  writes:
> On 2023-05-19 09:03, Michael Paquier wrote:
>> FYI, the buildfarm is seeing some spurious failures as well:
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer&dt=2023-05-1904%3A29%3A42

> Yes, it is the same error. Here's another one in version 13:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer&dt=2023-05-18%2022:37:49

Right.  I went ahead and pushed the fix in hopes of stabilizing things.
(I went with "trans_abc" as the new table name, for consistency with
some other nearby names.)

regards, tom lane

Re: smgrzeroextend clarification

[Peter Eisentraut]

I have committed some of the unrelated formatting changes separately, so 
what's left now is attached.


On 17.05.23 01:38, Andres Freund wrote:

- I left this for smgrzeroextend():

FIXME: why both blocknum and nblocks


I guess you're suggesting that we would do an lstat() to figure out the size
instead? Or use some cached size? That'd not be trivial to add - and it just
seems unrelated to smgzerorextend(), it's just as true for smgrextend().


What smgzerorextend() does now seems sensible to me.  I'd just like one 
or two sentences that document its API.  Right now, blocknum and nblocks 
are not documented at all.  Of course, we can guess, but I'm also 
interested in edge cases.  What are you supposed to pass for blocknum? 
What happens if it's not right after the current last block?  You 
answered that, but is that just what happens to happen, or do we 
actually want to support that?  What happens if blocknum is *before* the 
currently last block?  Would that overwrite the last existing blocks 
with zero?  What if nblocks is negative?  Does that zero out blocks 
backwards?


Obviously, the answer for most of these right now is that you're not 
supposed to do that.  But as the smgrextend() + hash index case shows, 
these things tend to grow in unexpected directions.


Also, slightly unrelated to the API, did we consider COW file systems? 
Like, is explicitly allocating blocks of zeroes sensible on btrfs?
From 17ffc6055c7755d110ca19f21bc328bf19197896 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 19 May 2023 16:49:03 +0200
Subject: [PATCH v2] WIP: Improve smgr source code comments

Discussion: 
https://www.postgresql.org/message-id/flat/22fed8ba-01c3-2008-a256-4ea912d68fab%40enterprisedb.com
---
 src/backend/storage/smgr/md.c   | 17 +
 src/backend/storage/smgr/smgr.c | 24 
 2 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 42e3501255..a19bab47f2 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -449,11 +449,11 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber 
forknum, bool isRedo)
 /*
  * mdextend() -- Add a block to the specified relation.
  *
- * The semantics are nearly the same as mdwrite(): write at the
- * specified position.  However, this is to be used for the case of
- * extending a relation (i.e., blocknum is at or beyond the current
- * EOF).  Note that we assume writing a block beyond current EOF
- * causes intervening file space to become filled with zeroes.
+ * This is to be used for the case of extending a relation (i.e., blocknum is
+ * at or beyond the current EOF).  Note that writing a block beyond current
+ * EOF must cause the intervening file space to become filled with zeroes.
+ * The POSIX file system APIs do that automatically, so we don't need to do
+ * anything about that.
  */
 void
 mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
@@ -516,9 +516,6 @@ mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber 
blocknum,
 
 /*
  * mdzeroextend() -- Add new zeroed out blocks to the specified relation.
- *
- * Similar to mdextend(), except the relation can be extended by multiple
- * blocks at once and the added blocks will be filled with zeroes.
  */
 void
 mdzeroextend(SMgrRelation reln, ForkNumber forknum,
@@ -800,10 +797,6 @@ mdread(SMgrRelation reln, ForkNumber forknum, BlockNumber 
blocknum,
 
 /*
  * mdwrite() -- Write the supplied block at the appropriate location.
- *
- * This is to be used only for updating already-existing blocks of a
- * relation (ie, those before the current EOF).  To extend a relation,
- * use mdextend().
  */
 void
 mdwrite(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index f76c4605db..c4c6c5a373 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -30,7 +30,9 @@
 
 /*
  * This struct of function pointers defines the API between smgr.c and
- * any individual storage manager module.  Note that smgr subfunctions are
+ * any individual storage manager module.
+ *
+ * Note that smgr subfunctions are
  * generally expected to report problems via elog(ERROR).  An exception is
  * that smgr_unlink should use elog(WARNING), rather than erroring out,
  * because we normally unlink relations during post-commit/abort cleanup,
@@ -104,8 +106,7 @@ static void smgrshutdown(int code, Datum arg);
 
 
 /*
- * smgrinit(), smgrshutdown() -- Initialize or shut down storage
- *  managers.
+ * smgrinit() -- Initialize storage managers.
  *
  * Note: smgrinit is called during backend startup (normal or standalone
  * case), *not* during postmaster start.  Therefore, any resources created
@@ -144,6 +145,8 @@ smgrshutdown(int code, Datum arg)
 /*
  * smgropen() -- Return an SMgrRelation object

Re: WAL Insertion Lock Improvements

[Bharath Rupireddy]

On Fri, May 19, 2023 at 12:24 PM Michael Paquier  wrote:
>
> On Thu, May 18, 2023 at 11:18:25AM +0530, Bharath Rupireddy wrote:
> > I think what I have so far seems more verbose explaining what a
> > barrier does and all that. I honestly think we don't need to be that
> > verbose, thanks to README.barrier.
>
> Agreed.  This file is a mine of information.
>
> > I simplified those 2 comments as the following:
> >
> >  * NB: pg_atomic_exchange_u64, having full barrier semantics will ensure
> >  * the variable is updated before releasing the lock.
> >
> >  * NB: pg_atomic_exchange_u64, having full barrier semantics will ensure
> >  * the variable is updated before waking up waiters.
> >
> > Please find the attached v7 patch.
>
> Nit.  These sentences seem to be worded a bit weirdly to me.  How
> about:
> "pg_atomic_exchange_u64 has full barrier semantics, ensuring that the
> variable is updated before (releasing the lock|waking up waiters)."

I get it. How about the following similar to what
ProcessProcSignalBarrier() has?

+ * Note that pg_atomic_exchange_u64 is a full barrier, so we're guaranteed
+ * that the variable is updated before waking up waiters.
+ */

+ * Note that pg_atomic_exchange_u64 is a full barrier, so we're guaranteed
+ * that the variable is updated before releasing the lock.
  */

Please find the attached v8 patch with the above change.

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com


v8-0001-Optimize-WAL-insertion-lock-acquisition-and-relea.patch
Description: Binary data

Re: PG 16 draft release notes ready

[Sehrope Sarkuni]

The intro in "E.1.3. Changes" says "... between PostgreSQL 15 and the
previous major release".

That should be "... between PostgreSQL __16__ ..." right?

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

How to ensure that SSPI support (Windows) enabled?

[Dimitry Markman]

Hi
I’m looking at config_default.pl file and I can see the line

gss   => undef,# --with-gssapi=

I was advised to use SSPI API that is built-in (windows) instead of MIT Kerberos

So what should I set and where to ensure that result PostgreSQL build will 
support SSPI?

Thanks in advance

Dimitry Markman

Re: Memory leak from ExecutorState context?

[Tomas Vondra]

On 5/19/23 00:27, Melanie Plageman wrote:
> On Wed, May 17, 2023 at 6:35 PM Jehan-Guillaume de Rorthais
>  wrote:
>>
>> On Wed, 17 May 2023 13:46:35 -0400
>> Melanie Plageman  wrote:
>>
>>> On Wed, May 17, 2023 at 07:10:08PM +0200, Jehan-Guillaume de Rorthais wrote:
 On Tue, 16 May 2023 16:00:52 -0400
 Melanie Plageman  wrote:
> ...
> There are some existing indentation issues in these files, but you can
> leave those or put them in a separate commit.

 Reindented in v9.

 I put existing indentation issues in a separate commit to keep the actual
 patches clean from distractions.
>>>
>>> It is a matter of opinion, but I tend to prefer the commit with the fix
>>> for the existing indentation issues to be first in the patch set.
>>
>> OK. moved in v10 patch set.
> 
> v10 LGTM.

Thanks!

I've pushed 0002 and 0003, after some general bikeshedding and minor
rewording (a bit audacious, admittedly).

I didn't push 0001, I don't think generally do separate pgindent patches
like this (I only run pgindent on large patches to ensure it doesn't
cause massive breakage, not separately like this, but YMMV).

Anyway, that's it for PG16. Let's see if we can do more in this area for
PG17.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: How to ensure that SSPI support (Windows) enabled?

[Tom Lane]

Dimitry Markman  writes:
> I’m looking at config_default.pl file and I can see the line
> gss   => undef,# --with-gssapi=
> I was advised to use SSPI API that is built-in (windows) instead of MIT 
> Kerberos
> So what should I set and where to ensure that result PostgreSQL build will 
> support SSPI?

SSPI != GSS.  SSPI support is always built in Windows builds, see
win32_port.h:

#define ENABLE_SSPI 1

(Perhaps not the best place for such a thing, but somebody put it there.)

regards, tom lane

Re: Memory leak from ExecutorState context?

[Tom Lane]

Tomas Vondra  writes:
> I didn't push 0001, I don't think generally do separate pgindent patches
> like this (I only run pgindent on large patches to ensure it doesn't
> cause massive breakage, not separately like this, but YMMV).

It's especially pointless when the main pgindent run for v16 is going
to happen today (as soon as I get done clearing out my other queue
items).

regards, tom lane

Re: How to ensure that SSPI support (Windows) enabled?

[Dimitry Markman]

Hi Tom,
thanks a lot for your super  fast answer 😊. I really appreciate that

I was asking our 3p library people how to add windows support to gss and they 
said that on windows we should use SSPI
I’m not really familiar with either gssapi or SSPI

I see that macOS has builtin support for gssapi, so all I need is to use 
–with-gssapi
On linux I use MIT Kerberos that we build in our 3p environment (only linux)
When I ask to build MIT Kerberos on windows that’s when I was advised simply to 
use SSPI

Thanks again

dm


From: Tom Lane 
Date: Friday, May 19, 2023 at 11:26 AM
To: Dimitry Markman 
Cc: pgsql-hackers@lists.postgresql.org 
Subject: Re: How to ensure that SSPI support (Windows) enabled?
Dimitry Markman  writes:
> I’m looking at config_default.pl file and I can see the line
> gss   => undef,# --with-gssapi=
> I was advised to use SSPI API that is built-in (windows) instead of MIT 
> Kerberos
> So what should I set and where to ensure that result PostgreSQL build will 
> support SSPI?

SSPI != GSS.  SSPI support is always built in Windows builds, see
win32_port.h:

#define ENABLE_SSPI 1

(Perhaps not the best place for such a thing, but somebody put it there.)

regards, tom lane

"38.10.10. Shared Memory and LWLocks" may require a clarification

[Aleksander Alekseev]

Hi,

While re-reading 38.10.10. Shared Memory and LWLocks [1] and the
corresponding code in pg_stat_statements.c I noticed that there are
several things that can puzzle the reader.

The documentation and the example suggest that LWLock* should be
stored within a structure in shared memory:

```
typedef struct pgssSharedState
{
LWLock   *lock;
/* ... etc ... */
} pgssSharedState;
```

... and initialized like this:

```
LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);

pgss = ShmemInitStruct("pg_stat_statements",
   sizeof(pgssSharedState),
   &found);

if (!found)
{
pgss->lock = &(GetNamedLWLockTranche("pg_stat_statements"))->lock;
/* ... */
}

/* ... */

LWLockRelease(AddinShmemInitLock);
```

It is not clear why placing LWLock* in a local process memory would be a bug.

On top of that the documentation says:

"""
To avoid possible race-conditions, each backend should use the LWLock
AddinShmemInitLock when connecting to and initializing its allocation
of shared memory
"""

However it's not clear when a race-condition may happen. The rest of
the text gives an overall impression that the shmem_startup_hook will
be called by postmaster once (unless an extension places several hooks
in series). Thus there is no real need to ackquire AddinShmemInitLock
and it should be safe to store LWLock* in local process memory. This
memory will be inherited from postmaster by child processes and the
overall memory usage is going to be the same due to copy-on-write.

Perhaps we should clarify this.

Thoughts?

[1]: https://www.postgresql.org/docs/15/xfunc-c.html#XFUNC-SHARED-ADDIN

-- 
Best regards,
Aleksander Alekseev

Re: How to ensure that SSPI support (Windows) enabled?

[Stephen Frost]

Greetings,

Please don't top-post.

* Dimitry Markman (dmark...@mathworks.com) wrote:
> I was asking our 3p library people how to add windows support to gss and they 
> said that on windows we should use SSPI

They're correct.

> I’m not really familiar with either gssapi or SSPI

Kerberos support is provided through SSPI on Windows.  On Linux and Unix
systems in general, it's provided through GSSAPI.  On the wire, the two
are (mostly) compatible.

> I see that macOS has builtin support for gssapi, so all I need is to use 
> –with-gssapi

On most Unix-based systems (and certainly for MacOS), you should be
installing MIT Kerberos and using that for your GSSAPI library.  The
GSSAPI library included with MacOS has not been properly maintained by
Apple and is woefully out of date and using it will absolutely cause you
undue headaches.

> On linux I use MIT Kerberos that we build in our 3p environment (only linux)

Yes, MIT Kerberos on Linux makes sense.

> When I ask to build MIT Kerberos on windows that’s when I was advised simply 
> to use SSPI

That's correct, you should be using SSPI on Windows is the vast majority
of cases.

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Should CSV parsing be stricter about mid-field quotes?

[Daniel Verite]

Joel Jacobson wrote:

> I understand its necessity for STDIN, given that the end of input needs to
> be explicitly defined.
> However, for files, we have a known file size and the end-of-file can be
> detected without the need for special markers.
> 
> Also, is the difference in how server-side COPY CSV is capable of dealing
> with \. but apparently not the client-side \COPY CSV documented somewhere?

psql implements the client-side "\copy table from file..." with
COPY table FROM STDIN ...

COPY FROM file CSV somewhat differs as your example shows,
but it still mishandle \. when unquoted. For instance, consider this
file to load with COPY  t FROM '/tmp/t.csv' WITH CSV
$ cat /tmp/t.csv
line 1
\.
line 3
line 4

It results in having only "line 1" being imported.


Best regards,
-- 
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

Re: The documentation for READ COMMITTED may be incomplete or wrong

[Tom Lane]

Amit Langote  writes:
> On Fri, May 19, 2023 at 8:57 AM Tom Lane  wrote:
> +* Initialize per-relation EPQ tuple states.  Result relations, if any,
> +* get marked as blocked; others as not-fetched.

> Would it be helpful to clarify that "blocked" means blocked for a
> given EvalPlanQual() cycle?

Probably best to put that with the data structure's comments.  I changed
those to look like

/*
 * relsubs_done[scanrelid - 1] is true if there is no EPQ tuple for this
 * target relation or it has already been fetched in the current scan of
 * this target relation within the current EvalPlanQual test.
 */
bool   *relsubs_done;

/*
 * relsubs_blocked[scanrelid - 1] is true if there is no EPQ tuple for
 * this target relation during the current EvalPlanQual test.  We keep
 * these flags set for all relids listed in resultRelations, but
 * transiently clear the one for the relation whose tuple is actually
 * passed to EvalPlanQual().
 */
bool   *relsubs_blocked;


> BTW, I didn't quite understand why EPQ involving resultRelations must
> behave in this new way but not the EPQ during LockRows?

LockRows doesn't have a bug: it always fills all the EPQ tuple slots
it's responsible for, and it doesn't use EvalPlanQual() anyway.
In the name of simplicity I kept the behavior exactly the same for
callers other than nodeModifyTable.

Perhaps replication/logical/worker.c could use a closer look here.
It's not entirely clear to me that the EPQ state it sets up is ever
used; but if it is I think it is okay as I have it here, because it
looks like those invocations always have just one result relation in
the plan, so there aren't any "extra" result rels that need to be
blocked.

regards, tom lane

Re: PG 16 draft release notes ready

[Bruce Momjian]

On Fri, May 19, 2023 at 07:31:40AM -0700, Nathan Bossart wrote:
> On Thu, May 18, 2023 at 04:49:47PM -0400, Bruce Momjian wrote:
> > I have completed the first draft of the PG 16 release notes.  You can
> > see the output here:
> > 
> > https://momjian.us/pgsql_docs/release-16.html
> > 
> > I will adjust it to the feedback I receive;  that URL will quickly show
> > all updates.
> 
> Thanks!
> 
> > Allow GRANT to give vacuum and analyze permission to users beyond the
> > table owner or superusers (Nathan Bossart)
> 
> This one was effectively reverted in favor of the MAINTAIN privilege.

Okay, removed.

> > Create a predefined role with permission to perform maintenance
> > operations (Nathan Bossart)
> 
> IMO this should also mention the grantable MAINTAIN privilege.
> Alternatively, the item above about granting vacuum/analyze privileges
> could be adjusted.

Very good point --- here is the new text:





Create a predefined role and grantable privilege with permission to 
perform maintenance operations (Nathan Bossart)



The predefined role is is called pg_maintain.



I will commit this change now.

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: PG 16 draft release notes ready

[Bruce Momjian]

On Fri, May 19, 2023 at 11:07:29AM -0400, Sehrope Sarkuni wrote:
> The intro in "E.1.3. Changes" says "... between PostgreSQL 15 and the previous
> major release".
> 
> That should be "... between PostgreSQL __16__ ..." right?

Oh, I thought I had changed all those --- fixed now, thanks!

-- 
  Bruce Momjian  https://momjian.us
  EDB  https://enterprisedb.com

  Only you can decide what is important to you.

Re: Conflict between regression tests namespace & transactions due to recent changes

[Marina Polyakova]

On 2023-05-19 17:59, Tom Lane wrote:

Marina Polyakova  writes:

On 2023-05-19 09:03, Michael Paquier wrote:

FYI, the buildfarm is seeing some spurious failures as well:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer&dt=2023-05-1904%3A29%3A42



Yes, it is the same error. Here's another one in version 13:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer&dt=2023-05-18%2022:37:49


Right.  I went ahead and pushed the fix in hopes of stabilizing things.
(I went with "trans_abc" as the new table name, for consistency with
some other nearby names.)

regards, tom lane


Thank you! I missed the same changes in version 11 :(

--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Missing warning on revokes with grant options

[Joseph Koshakow]

I've been thinking about this some more and reading the SQL99 spec. In
the original thread that added these warnings [0], which was linked
earlier in this thread by Nathan, the following assertion was made:

> After that, you get to the General Rules, which pretty clearly say that
> trying to grant privileges you don't have grant option for is just a
> warning and not an error condition.  (Such privileges will not be in the
> set of "identified privilege descriptors".)
>
> AFAICS the specification for REVOKE is exactly parallel.

I think it is true that for both GRANT and REVOKE, if a privilege was
specified in the statement and a corresponding privilege does not exist
in the identified set then a warning should be issued. However, the
meaning of "identified set" is different between GRANT and REVOKE.

In GRANT the identified set is defined as

4) A set of privilege descriptors is identified. The privilege
descriptors identified are those defining,
for each  explicitly or implicitly in , that
 on O held by A with
grant option.

Essentially it is all privileges specified in the GRANT statement on O
**where by A is the grantee with a grant option**.

In REVOKE the identified set is defined as

1) Case:
  a) If the  is a , then
for every 
 specified, a set of privilege descriptors is identified. A
privilege descriptor P is said to be
 identified if it belongs to the set of privilege descriptors that
defined, for any 
 explicitly or implicitly in , that  on O, or
any of the objects in S, granted
 by A to .

Essentially it is all privileges specified in the REVOKE statement on O
**where A is the grantor and the grantee is one of the grantees
specified in the REVOKE statement**.

In fact as far as I can tell, the ability to revoke a privilege does
not directly depend on having a grant option for that privilege, it
only depends on being the grantor of the specified privilege. However,
our code in restrict_and_check_grant doesn't match this. It treats the
rules for GRANTs and REVOKEs the same, in that you need a grant option
to execute either. It's possible that due to the abandoned privilege
rules that it is impossible for a privilege to exist where the grantor
doesn't also have a grant option on that privilege. I haven't read that
part of the spec closely enough.

As a consequence of how the identified set is defined for REVOKE, not
only should a warning be issued in the example from my previous email,
but I think a warning should also be issued even if the grantee has no
privileges on O. For example,

```
test=# SELECT current_role;
 current_role
--
 joe
(1 row)

test=# CREATE TABLE t ();
CREATE TABLE
test=# CREATE ROLE r1;
CREATE ROLE
test=# SELECT relacl FROM pg_class WHERE relname = 't';
 relacl


(1 row)

test=# REVOKE SELECT ON t FROM r1;
REVOKE
```

Here the identified set for the REVOKE statement is empty. So there is
no corresponding privilege descriptor in the identified set for the
SELECT privilege in the REVOKE statement. So a warning should be
issued. Recall:

18) If the  is a , then:
  a) For every combination of  and  on O specified in
, if there
 is no corresponding privilege descriptor in the set of identified
privilege descriptors, then a
 completion condition is raised: warning — privilege not revoked

Essentially the meaning of the warning for REVOKE does not mean "you
tried to revoke a privilege but you don't have a grant option", it
means "you tried to revoke a privilege (where you are the grantor), but
such a privilege does not exist".

Thanks,
Joe Koshakow

[0] https://postgr.es/m/20040511091816.E9887CF519E%40www.postgresql.com

Re: PG 16 draft release notes ready

[jian he]

On Fri, May 19, 2023 at 4:49 AM Bruce Momjian  wrote:

> I have completed the first draft of the PG 16 release notes.  You can
> see the output here:
>
> https://momjian.us/pgsql_docs/release-16.html
>
> I will adjust it to the feedback I receive;  that URL will quickly show
> all updates.
>
> I learned a few things creating it this time:
>
> *  I can get confused over C function names and SQL function names in
>commit messages.
>
> *  The sections and ordering of the entries can greatly clarify the
>items.
>
> *  The feature count is slightly higher than recent releases:
>
> release-10:  189
> release-11:  170
> release-12:  180
> release-13:  178
> release-14:  220
> release-15:  184
> --> release-16:  200
>
> --
>   Bruce Momjian  https://momjian.us
>   EDB  https://enterprisedb.com
>
>   Only you can decide what is important to you.
>
>
>

Add function pg_dissect_walfile_name() to report the segment and timeline
> values of WAL file names (Bharath Rupireddy)


https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=13e0d7a603852b8b05c03b45228daabffa0cced2
the function rename to pg_split_walfile_name.

seems didn't mention pg_input_is_valid,pg_input_error_info?
https://www.postgresql.org/docs/devel/functions-info.html#FUNCTIONS-INFO-VALIDITY-TABLE

Re: New COPY options: DELIMITER NONE and QUOTE NONE

[Andrew Dunstan]

On 2023-05-19 Fr 05:24, Joel Jacobson wrote:


I hacked together a broken patch just to demonstrate the idea on syntax
and basic idea. The `COPY ... FROM` examples above works.
But it doesn't work at all for `COPY ... TO`, since it output \0 byte as
delimiter and quote in the output, which is of course not what we want.




I think you've been a bit too cute with the grammar changes, but as you 
say this is a POC.



cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: Adding SHOW CREATE TABLE

[Andrew Dunstan]

On 2023-05-18 Th 19:53, Stephen Frost wrote:

Greetings,

* Kirk Wolak (wol...@gmail.com) wrote:

My approach for now is to develop this as the \st command.
After reviewing the code/output from the 3 sources (psql, fdw, and
pg_dump).  This trivializes the approach,
and requires the smallest set of changes (psql is already close with
existing queries, etc).

And frankly, I would rather have an \st feature that handles 99% of the use
cases then go 15yrs waiting for a perfect solution.
Once this works well for the group.  Then, IMO, that would be the time to
discuss moving it.

Having this only available via psql seems like the least desirable
option as then it wouldn't be available to any other callers..

Having it in libpq, on the other hand, would make it available to psql
as well as any other utility, library, or language / driver which uses
libpq, including pg_dump..

Using libpq would also make sense from the perspective that libpq can be
used to connect to a number of different major versions of PG and this
could work work for them all in much the way that pg_dump does.

The downside with this apporach is that drivers which don't use libpq
(eg: JDBC) would have to re-implement this if they wanted to keep
feature parity with libpq..



I think the ONLY place we should have this is in server side functions. 
More than ten years ago I did some work in this area (see below), but 
it's one of those things that have been on my ever growing personal TODO 
list


See  and 




cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Re: psql: Could we get "-- " prefixing on the **** QUERY **** outputs? (ECHO_HIDDEN)

[Alvaro Herrera]

On 2023-May-19, Andrey M. Borodin wrote:

> I have a question, but mostly for my own knowledge. Translation
> changes seem trivial for all languages, do we typically fix .po files
> in such cases? Or do we leave it to translators to revise the stuff?

The translations use a completely separate source repository, so even if
somebody were to patch them in postgresql.git, their changes would be
overwritten next time they are copied from the translation repo anyway.
Just leave it to the translators.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"In Europe they call me Niklaus Wirth; in the US they call me Nickel's worth.
 That's because in Europe they call me by name, and in the US by value!"

Re: Missing warning on revokes with grant options

[Joseph Koshakow]

Sorry for the multiple consecutive emails. I just came across this
comment that explains the current behavior in restrict_and_check_grant

/*
* Restrict the operation to what we can actually grant or revoke, and
* issue a warning if appropriate.  (For REVOKE this isn't quite what the
* spec says to do: the spec seems to want a warning only if no privilege
* bits actually change in the ACL. In practice that behavior seems much
* too noisy, as well as inconsistent with the GRANT case.)
*/

However, I still think the current behavior is a bit strange since
holding a grant option is not directly required to issue a revoke.
Perhaps for revoke the logic should be:
  - for each specified privilege:
  - if the set of acl items on the specified object that includes
this privilege is non empty
  - and none of those acl items have the current role as the
grantor
  - then issue a warning.

Thanks,
Joe Koshakow

Re: The documentation for READ COMMITTED may be incomplete or wrong

[Tom Lane]

Amit Langote  writes:
> On Fri, May 19, 2023 at 8:57 AM Tom Lane  wrote:
>> What I'm thinking about doing to back-patch this is to replace
>> one of the pointer fields in EPQState with a pointer to a
>> subsidiary palloc'd structure, where we can put the new fields
>> along with the cannibalized old one.  We've done something
>> similar before, and it seems a lot safer than having basically
>> different logic in v16 than earlier branches.

> +1.

Done that way.  I chose to replace the tuple_table field, because
it was in a convenient spot and it seemed like the field least
likely to have any outside code referencing it.

regards, tom lane

Re: The documentation for READ COMMITTED may be incomplete or wrong

[Aleksander Alekseev]

Hi Tom,

> Done that way.  I chose to replace the tuple_table field, because
> it was in a convenient spot and it seemed like the field least
> likely to have any outside code referencing it.

Many thanks!

If it's not too much trouble could you please recommend good entry
points to learn more about the internals of this part of the system
and accompanying edge cases? Perhaps there is an experiment or two an
extension author can do in order to lower the entry threshold and/or
known bugs, limitations or wanted features one could start with?

-- 
Best regards,
Aleksander Alekseev

Re: Order changes in PG16 since ICU introduction

[Daniel Verite]

Jeff Davis wrote:

> 2) Automatically change the provider to libc when locale=C.
> 
> Almost works, but it's not clear how we handle the case "provider=icu
> lc_collate='fr_FR.utf8' locale=C".
> 
> If we change it to "provider=libc lc_collate=C", we've overridden the
> specified lc_collate. If we ignore the locale=C, that would be
> surprising to users. If we throw an error, that would be a backwards
> compatibility issue.

This thread started with a report illustrating that when users mention
the locale "C", they implicitly mean "C" from the libc provider, as
when libc was the default. The problem is that as soon as ICU is the
default, any reference to a libc collation should mention explicitly
that the provider is libc.

It seems what we're set on the idea to create an exception for "C"
(and I assume also "POSIX") to avoid too much confusion, and because
"C" is quite special anyway, and has no equivalent in ICU (the switch
in v16 to ICU as the default provider is based on the premise that the
locales with the same name will behave pretty much the same with ICU
as they did with libc, but it's absolutely not the case with "C").

ISTM that if we want to go that route, we need the make the minimum
changes at the user interface level and not any deeper, so that when
(locale="C" OR locale="POSIX") AND the provider has not been specified,
then the command (initdb and create database) act as if the user had
specified provider=libc.


> (3) Support iculocale=C in the ICU provider using the memcmp() path.

> In other words, if provider=icu and iculocale=C, lc_collate_is_c() and
> lc_ctpye_is_c() would both return true.

ICU does not provide a locale that behaves like that, and it doesn't
feel right to pretend it does. It feels like attacking the problem
at the wrong level.

> (4) Create a new "none" provider (which has no locale and always memcmp
> semantics), and automatically change the provider to "none" if
> provider=icu and iculocale=C.

It still uses libc/C for character classification and case changing,
so "no locale" is technically not true. Personally I don't see
the benefit of adding a "none" provider. C is a libc locale
and libc is not disappearing. I also think  that when users explicitly
indicate provider=icu, they should get icu.


Best regards,
-- 
Daniel Vérité
https://postgresql.verite.pro/
Twitter: @DanielVerite

Re: Assert failure of the cross-check for nullingrels

[Tom Lane]

Richard Guo  writes:
> I keep thinking about my proposal in v2 patch.  It seems more natural to
> me to fix this issue, because an outer join's quals are always treated
> as a whole when we check if identity 3 applies in make_outerjoininfo, as
> well as when we adjust the outer join's quals for commutation in
> deconstruct_distribute_oj_quals.

No, I doubt that that patch works properly.  If the join condition
contains independent quals on different relations, say

select ... from t1 left join t2 on (t1.a = 1 and t2.b = 2)

then it may be that those quals need to be pushed to different levels.
I don't believe that considering the union of the rels mentioned in
any qual is a reasonable thing to do here.

regards, tom lane

Re: How to ensure that SSPI support (Windows) enabled?

[Dimitry Markman]

Thanks Stephen, very useful information
dm


On 5/19/23, 12:02 PM, "Stephen Frost"  wrote:
Greetings,

Please don't top-post.

* Dimitry Markman (dmark...@mathworks.com) wrote:
> I was asking our 3p library people how to add windows support to gss and they 
> said that on windows we should use SSPI

They're correct.

> I’m not really familiar with either gssapi or SSPI

Kerberos support is provided through SSPI on Windows.  On Linux and Unix
systems in general, it's provided through GSSAPI.  On the wire, the two
are (mostly) compatible.

> I see that macOS has builtin support for gssapi, so all I need is to use 
> –with-gssapi

On most Unix-based systems (and certainly for MacOS), you should be
installing MIT Kerberos and using that for your GSSAPI library.  The
GSSAPI library included with MacOS has not been properly maintained by
Apple and is woefully out of date and using it will absolutely cause you
undue headaches.

> On linux I use MIT Kerberos that we build in our 3p environment (only linux)

Yes, MIT Kerberos on Linux makes sense.

> When I ask to build MIT Kerberos on windows that’s when I was advised simply 
> to use SSPI

That's correct, you should be using SSPI on Windows is the vast majority
of cases.

Thanks,

Stephen

Re: Add operator for dividing interval by an interval

[Andres Freund]

Hi,

On 2023-05-18 17:03:24 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > What about an interval / interval -> double operator that errors out 
> > whenever
> > month is non-zero? As far as I can tell that would always be deterministic.
>
> We have months, days, and microseconds, and microseconds-per-day isn't
> much more stable than days-per-month (because DST).

I was about to counter that, if you subtract a timestamp before/after DST
changes, you currently don't get a full day for the "shorter day":

SET timezone = 'America/Los_Angeles';
SELECT '2023-03-13 23:00:00-07'::timestamptz - '2023-03-11 
23:00:00-08'::timestamptz;
┌┐
│?column?│
├┤
│ 1 day 23:00:00 │
└┘

which afaics would make it fine to just use 24h days when dividing intervals.


However, that seems to lead to quite broken results:

SET timezone = 'America/Los_Angeles';
WITH s AS (SELECT '2023-03-11 23:00-08'::timestamptz a, '2023-03-13 
23:00-07'::timestamptz b) SELECT a, b, b - a AS b_min_a, a + (b - a) FROM s;
┌┬┬┬┐
│   a│   b│b_min_a │
?column?│
├┼┼┼┤
│ 2023-03-11 23:00:00-08 │ 2023-03-13 23:00:00-07 │ 1 day 23:00:00 │ 2023-03-13 
22:00:00-07 │
└┴┴┴┘


I subsequently found a comment that seems to reference this in timestamp_mi().
/*--
 *  This is wrong, but removing it breaks a lot of regression tests.
 *  For example:
 *


How's this not a significant bug that we need to fix?


I'm not sure this ought to be fixed in timestamp_mi() - perhaps the order of
operations in timestamp_pl_interval() would be a better place?


We probably should document that interval math isn't associative:

postgres[2807421][1]=# SELECT ('2023-03-11 23:00:00-08'::timestamptz + '1 
day'::interval) + '23h'::interval;
┌┐
│?column?│
├┤
│ 2023-03-13 22:00:00-07 │
└┘

postgres[2807421][1]=# SELECT ('2023-03-11 23:00:00-08'::timestamptz + 
'23h'::interval) + '1day'::interval;
┌┐
│?column?│
├┤
│ 2023-03-13 23:00:00-07 │
└┘


Greetings,

Andres Freund

Re: Adding SHOW CREATE TABLE

[Corey Huinker]

>
> I think the ONLY place we should have this is in server side functions.
> More than ten years ago I did some work in this area (see below), but it's
> one of those things that have been on my ever growing personal TODO list
>
> See 
>  and
> 
> 
>
>
>
> Some additional backstory, as this has come up before...

A direction discussion of a previous SHOW CREATE effort:
https://www.postgresql.org/message-id/20190705163203.gd24...@fetter.org

Other databases' implementations of SHOW CREATE came up in this discussion
as well:
https://www.postgresql.org/message-id/CADkLM=fxfsrhaskk_by_a4uomj1te5mfggd_rwwqfv8wp68...@mail.gmail.com

Clearly, there is customer demand for something like this, and has been for
a long time.

Re: Adding SHOW CREATE TABLE

[Laurenz Albe]

On Fri, 2023-05-19 at 13:08 -0400, Andrew Dunstan wrote:
> On 2023-05-18 Th 19:53, Stephen Frost wrote:
> > * Kirk Wolak (wol...@gmail.com) wrote:
> > > My approach for now is to develop this as the \st command.
> > > After reviewing the code/output from the 3 sources (psql, fdw, and
> > > pg_dump).
> >
> > Having this only available via psql seems like the least desirable
> > option as then it wouldn't be available to any other callers..
> > 
> > Having it in libpq, on the other hand, would make it available to psql
> > as well as any other utility, library, or language / driver which uses
> > libpq, including pg_dump..
> 
> I think the ONLY place we should have this is in server side functions.

+1

A function "pg_get_tabledef" would blend nicely into the following list:

\df pg_get_*def
   List of functions
   Schema   │  Name  │ Result data type │  Argument 
data types  │ Type 
╪╪══╪═══╪══
 pg_catalog │ pg_get_constraintdef   │ text │ oid   
│ func
 pg_catalog │ pg_get_constraintdef   │ text │ oid, boolean  
│ func
 pg_catalog │ pg_get_functiondef │ text │ oid   
│ func
 pg_catalog │ pg_get_indexdef│ text │ oid   
│ func
 pg_catalog │ pg_get_indexdef│ text │ oid, integer, 
boolean │ func
 pg_catalog │ pg_get_partition_constraintdef │ text │ oid   
│ func
 pg_catalog │ pg_get_partkeydef  │ text │ oid   
│ func
 pg_catalog │ pg_get_ruledef │ text │ oid   
│ func
 pg_catalog │ pg_get_ruledef │ text │ oid, boolean  
│ func
 pg_catalog │ pg_get_statisticsobjdef│ text │ oid   
│ func
 pg_catalog │ pg_get_triggerdef  │ text │ oid   
│ func
 pg_catalog │ pg_get_triggerdef  │ text │ oid, boolean  
│ func
 pg_catalog │ pg_get_viewdef │ text │ oid   
│ func
 pg_catalog │ pg_get_viewdef │ text │ oid, boolean  
│ func
 pg_catalog │ pg_get_viewdef │ text │ oid, integer  
│ func
 pg_catalog │ pg_get_viewdef │ text │ text  
│ func
 pg_catalog │ pg_get_viewdef │ text │ text, boolean 
│ func
(17 rows)


A server function can be conveniently called from any client code.

Yours,
Laurenz Albe

Re: Adding SHOW CREATE TABLE

[Ziga]

On 19/05/2023 19:08, Andrew Dunstan wrote:
I think the ONLY place we should have this is in server side 
functions. More than ten years ago I did some work in this area (see 
below), but it's one of those things that have been on my ever growing 
personal TODO list


See  and 



I have my own implementation of SHOW CREATE as a ddlx extension 
available at https://github.com/lacanoid/pgddl


Far from perfect but getting better and it seems good enough to be used 
by some people...


Ž.

Re: benchmark results comparing versions 15.2 and 16

[MARK CALLAGHAN]

On Tue, May 9, 2023 at 10:36 AM MARK CALLAGHAN  wrote:

>
>
> On Fri, May 5, 2023 at 10:01 PM MARK CALLAGHAN  wrote:
>
>> I have two more runs of the benchmark in progress so we will have 3
>> results for each of the test cases to confirm that the small regressions
>> are repeatable.
>>
>
I repeated the benchmark a few times using a more recent PG16 build (git
sha 08c45ae2) and have yet to see any significant changes. So that is good
news. My testing scripts have been improved so I should be able to finish
the next round of tests in less time.

--
Mark Callaghan
mdcal...@gmail.com

Re: Order changes in PG16 since ICU introduction

[Tom Lane]

Jeff Davis  writes:
> Committed, thank you.

This commit has given the PDF docs build some indigestion:

Making portrait pages on A4 paper (210mmx297mm)
/home/postgres/bin/fop -fo postgres-A4.fo -pdf postgres-A4.pdf
[WARN] FOUserAgent - Font "Symbol,normal,700" not found. Substituting with 
"Symbol,normal,400".
[WARN] FOUserAgent - Font "ZapfDingbats,normal,700" not found. Substituting 
with "ZapfDingbats,normal,400".
[WARN] FOUserAgent - Hyphenation pattern not found. URI: en.
[WARN] FOUserAgent - The contents of fo:block line 1 exceed the available area 
in the inline-progression direction by 3531 millipoints. (See position 
55117:2388)
[WARN] FOUserAgent - The contents of fo:block line 1 exceed the available area 
in the inline-progression direction by 1871 millipoints. (See position 
55117:12998)
[WARN] FOUserAgent - Glyph "?" (0x323, dotbelowcmb) not available in font 
"Courier".
[WARN] FOUserAgent - Glyph "?" (0x302, circumflexcmb) not available in font 
"Courier".
[WARN] FOUserAgent - The contents of fo:block line 12 exceed the available area 
in the inline-progression direction by 20182 millipoints. (See position 
55172:188)
[WARN] FOUserAgent - The contents of fo:block line 10 exceed the available area 
in the inline-progression direction by 17682 millipoints. (See position 
55172:188)
[WARN] FOUserAgent - Glyph "?" (0x142, lslash) not available in font 
"Times-Roman".
[WARN] PropertyMaker - span="inherit" on fo:block, but no explicit value found 
on the parent FO.

(The first three and last one warnings are things we've been living
with, but the ones between are new.)

The first two "exceed the available area" complaints are in the "ICU
Collation Levels" table.  We can silence them by providing some column
width hints to make the "Description" column a tad wider than the rest,
as in the proposed patch attached.  The other two, as well as the first
two glyph-not-available complaints, are caused by this bit:

   Full normalization is important in some cases, such as when
   multiple accents are applied to a single character. For instance,
   'ệ' can be composed of code points
   U&'\0065\0323\0302' or
   U&'\0065\0302\0323'. With full normalization
   on, these code point sequences are treated as equal; otherwise they
   are unequal.

which renders just abysmally (see attached screen shot), and even in HTML
where it's rendering about as intended, it really is an unintelligible
example.  Few people are going to understand that the circumflex and the
dot-below are separately applied accents.  How about we drop the explicit
example and write something like

   Full normalization allows code point sequences such as
   characters with multiple accent marks applied in different
   orders to be seen as equal.

?

(The last missing-glyph complaint is evidently from the release notes;
I'll bug Bruce about that separately.)

regards, tom lane

diff --git a/doc/src/sgml/charset.sgml b/doc/src/sgml/charset.sgml
index 9db14649aa..96a23bf530 100644
--- a/doc/src/sgml/charset.sgml
+++ b/doc/src/sgml/charset.sgml
@@ -1140,6 +1140,14 @@ SELECT 'w;x*y-z' = 'wxyz' COLLATE num_ignore_punct; -- true
  
   ICU Collation Levels
   
+   
+   
+   
+   
+   
+   
+   
+   

 
  Level

Re: benchmark results comparing versions 15.2 and 16

[Andres Freund]

Hi,

On 2023-05-19 15:44:09 -0700, MARK CALLAGHAN wrote:
> On Tue, May 9, 2023 at 10:36 AM MARK CALLAGHAN  wrote:
> 
> >
> >
> > On Fri, May 5, 2023 at 10:01 PM MARK CALLAGHAN  wrote:
> >
> >> I have two more runs of the benchmark in progress so we will have 3
> >> results for each of the test cases to confirm that the small regressions
> >> are repeatable.
> >>
> >
> I repeated the benchmark a few times using a more recent PG16 build (git
> sha 08c45ae2) and have yet to see any significant changes. So that is good
> news. My testing scripts have been improved so I should be able to finish
> the next round of tests in less time.

With "yet to see any significant changes" do you mean that the runs are
comparable with earlier runs, showing the same regression? Or that the
regression vanished? Or ...?

Greetings,

Andres Freund

Re: PG 16 draft release notes ready

[Tom Lane]

The v16 release notes are problematic in a PDF docs build:

[WARN] FOUserAgent - Glyph "?" (0x142, lslash) not available in font 
"Times-Roman".

This is evidently from

Add functions to add, subtract, and generate timestamptz values in a specified 
time zone (Przemysław Sztoch, Gurjeet Singh)

Change date_trunc(unit, timestamptz, time_zone) to be an immutable function 
(Przemysław Sztoch)

since "ł" doesn't exist in ISO8859-1.  I'd suggest dumbing these
down to plain "l".

regards, tom lane

Re: Wrong results from Parallel Hash Full Join

[Tom Lane]

I noticed that BF animal conchuela has several times fallen over on the
test case added by 558c9d75f:

diff -U3 
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/expected/join_hash.out 
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/results/join_hash.out
--- 
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/expected/join_hash.out   
2023-04-19 10:20:26.15984 +0200
+++ 
/home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/results/join_hash.out
2023-04-19 10:21:47.97190 +0200
@@ -974,8 +974,8 @@
 SELECT * FROM hjtest_matchbits_t1 t1 FULL JOIN hjtest_matchbits_t2 t2 ON t1.id 
= t2.id;
  id | id 
 +
-  1 |   
 |  2
+  1 |   
 (2 rows)
 
 -- Test serial full hash join.

Considering that this is a parallel plan, I don't think there's any
mystery about why an ORDER-BY-less query might have unstable output
order; the only mystery is why more of the buildfarm hasn't failed.
Can we just add "ORDER BY t1.id" to this query?  It looks like you
get the same PHJ plan, although now underneath Sort/Gather Merge.

regards, tom lane

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=conchuela&dt=2023-04-19%2008%3A20%3A56
[2] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=conchuela&dt=2023-05-03%2006%3A21%3A03
[3] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=conchuela&dt=2023-05-19%2022%3A21%3A04

Avoiding another needless ERROR during nbtree page deletion

[Peter Geoghegan]

Work from commit 5b861baa (later backpatched as commit 43e409ce)
taught nbtree to press on with vacuuming an index when page deletion
fails to "re-find" a downlink in the target page's parent (or in some
page to the right of the parent) due to index corruption.

To recap, avoiding ERRORs during vacuuming (even those caused by index
corruption) is useful because there is no reason to expect the error
to go away on its own; we're relying on the DBA to notice the error
and REINDEX before wraparound/xidStopLimit kicks in. This is at least
the case on versions before 14, where the failsafe can eventually
kick-in and avoid catastrophe (though the failsafe can only be
expected to avoid the worst consequences).

It has come to my attention that there is a remaining issue of the
same general nature in nbtree VACUUM's page deletion code. Though this
remaining issue seems significantly less likely to come up in
practice, there is no reason to take any chances here. Attached patch
fixes it.

Also attached is a bugfix for a minor issue in amcheck's
bt_index_parent_check() function, which I noticed in passing, while I
tested the first patch. We assumed that we'd always land on the
leftmost page on each level first (the leftmost according to internal
pages one level up). That assumption is faulty because page deletion
of the leftmost page is quite possible. Page deletion can be
interrupted, leaving a half-dead leaf page (possibly the leftmost leaf
page) without any downlink one level up, while still leaving a left
sibling link on the leaf level (in the leaf page that isn't about to
become the leftmost, but won't until the interrupted page deletion can
be completed).

IMV this should be backpatched all the way. The issue in question is
rather unlikely to come up. But the fix that I've come up with is very
well targeted. It seems just about impossible for it to affect any
user that didn't already have a serious problem (without the fix).

-- 
Peter Geoghegan


v1-0001-nbtree-VACUUM-cope-with-right-sibling-link-corrup.patch
Description: Binary data


v1-0002-amcheck-Fix-bt_index_parent_check-false-positive.patch
Description: Binary data

Re: PG 16 draft release notes ready

[jian he]

Sorry for changing the subject line.

these two commits seems not mentioned.
Fix ts_headline() edge cases for empty query and empty search text.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=029dea882a7aa34f46732473eed7c917505e6481

Simplify the implementations of the to_reg* functions.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3ea7329c9a79ade27b5d3742d1a41ce6d0d9aca8

Re: PG 16 draft release notes ready

[Isaac Morland]

On Fri, 19 May 2023 at 22:59, jian he  wrote:

>
> Sorry for changing the subject line.
>
> these two commits seems not mentioned.
>

On a similar topic, should every committed item from the commitfest be
mentioned, or only ones that are significant enough?

I’m wondering because I had a role in this very small item, yet I don’t see
it listed in the psql section:

https://commitfest.postgresql.org/42/4133/

It’s OK if we don’t mention every single change, I just want to make sure I
understand.