Re: Initial Schema Sync for Logical Replication

[Amit Kapila]

On Mon, May 22, 2023 at 6:37 AM Masahiko Sawada  wrote:
>
> On Fri, Apr 28, 2023 at 4:16 PM Masahiko Sawada  wrote:
> > Yes, in this approach, we need to dump/restore objects while
> > specifying with fine granularity. Ideally, the table sync worker dumps
> > and restores the table schema, does copy the initial data, and then
> > creates indexes, and triggers and table-related objects are created
> > after that. So if we go with the pg_dump approach to copy the schema
> > of individual tables, we need to change pg_dump (or libpgdump needs to
> > be able to do) to support it.
>
> We have been discussing how to sync schema but I'd like to step back a
> bit and discuss use cases and requirements of this feature.
>
> Suppose that a table belongs to a publication, what objects related to
> the table we want to sync by the initial schema sync features? IOW, do
> we want to sync table's ACLs, tablespace settings, triggers, and
> security labels too?
>
> If we want to replicate the whole database, e.g. when using logical
> replication for major version upgrade, it would be convenient if it
> synchronizes all table-related objects. However, if we have only this
> option, it could be useless in some cases. For example, in a case
> where users have different database users on the subscriber than the
> publisher, they might want to sync only CREATE TABLE, and set ACL etc
> by themselves. In this case, it would not be necessary to sync ACL and
> security labels.
>
> What use case do we want to support by this feature? I think the
> implementation could be varied depending on how to select what objects
> to sync.
>
> One possible idea is to select objects to sync depending on how DDL
> replication is set in the publisher. It's straightforward but I'm not
> sure the design of DDL replication syntax has been decided. Also, even
> if we create a publication with ddl = 'table' option, it's not clear
> to me that we want to sync table-dependent triggers, indexes, and
> rules too by the initial sync feature.
>

I think it is better to keep the initial sync the same as the
replication. So, if the publication specifies 'table' then we should
just synchronize tables. Otherwise, it will look odd that the initial
sync has synchronized say index-related DDLs but then later
replication didn't replicate it. OTOH, if we want to do initial sync
of table-dependent objects like triggers, indexes, rules, etc. when
the user has specified ddl = 'table' then the replication should also
follow the same. The main reason to exclude the other objects during
replication is to reduce the scope of deparsing patch but if we have a
finite set of objects (say all dependent on the table) then we can
probably try to address those.

> Second idea is to make it configurable by users so that they can
> specify what objects to sync. But it would make the feature complex
> and I'm not sure users can use it properly.
>
> Third idea is that since the use case of synchronizing the whole
> database can be achievable even by pg_dump(all), we support
> synchronizing only tables (+ indexes) in the initial sync feature,
> which can not be achievable by pg_dump.
>

Can't we add some switch to dump only the table and not its dependents
if we want to go with that approach?

-- 
With Regards,
Amit Kapila.

Re: PG 16 draft release notes ready

[John Naylor]

On Tue, May 23, 2023 at 11:26 AM Bruce Momjian  wrote:
>
> On Tue, May 23, 2023 at 09:58:30AM +0700, John Naylor wrote:
> > > Allow ASCII string detection to use vector operations on x86-64
architectures
> > (John Naylor)
> > > Allow JSON string processing to use vector operations on x86-64
architectures
> > (John Naylor)
> > >
> > > ARM?
> >
> > Arm as well. For anything using 16-byte vectors the two architectures
are
> > equivalently supported. For all the applications, I would just say
"vector" or
> > "SIMD".
>
> Okay, I kept "vector".  I don't think moving them into performance makes
> sense because there I don't think this would impact user behavior or
> choice, and it can't be controlled.

Well, these two items were only committed because of measurable speed
increases, and have zero effect on how developers work with "source code",
so that's a category error.

Whether they rise to the significance of warranting inclusion in release
notes is debatable.

> > > Allow xid/subxid searches to use vector operations on x86-64
architectures
> > (Nathan Bossart)
> >
> > When moved to the performance section, it would be something like
"improve
> > scalability when a large number of write transactions are in progress".
>
> Uh, again, see above, this does not impact user behavior or choices.

So that turns a scalability improvement into "source code"?

> I assume this is x86-64-only.

Au contraire, I said "For anything using 16-byte vectors the two
architectures are equivalently supported". It's not clear from looking at
individual commit messages, that's why I piped in to help.

--
John Naylor
EDB: http://www.enterprisedb.com

Re: PG 16 draft release notes ready

[Bruce Momjian]

On Tue, May 23, 2023 at 09:58:30AM +0700, John Naylor wrote:
> Hi Bruce,
> 
> > Add support for SSE2 (Streaming SIMD Extensions 2) vector operations on
> x86-64 architectures (John Naylor)
> 
> > Add support for Advanced SIMD (Single Instruction Multiple Data) (NEON)
> instructions on ARM architectures (Nathan Bossart)
> 
> Nit: It's a bit odd that SIMD is spelled out in only the Arm entry, and 
> perhaps
> expanding the abbreviations can be left out.

The issue is that x86-64's SSE2 uses an embedded acronym:

SSE2 (Streaming SIMD Extensions 2)

so technically it is:

SSE2 (Streaming (Single Instruction Multiple Data) Extensions 2

but embedded acronyms is something I wanted to avoid.  ;-)

> > Allow arrays searches to use vector operations on x86-64 architectures (John
> Naylor)
> 
> We can leave out the architecture here (see below). Typo: "array searches"

Both fixed.

> All the above seem appropriate for the "source code" section, but the 
> following
> entries might be better in the "performance" section:
> 
> > Allow ASCII string detection to use vector operations on x86-64 
> > architectures
> (John Naylor)
> > Allow JSON string processing to use vector operations on x86-64 
> > architectures
> (John Naylor)
> >
> > ARM?
> 
> Arm as well. For anything using 16-byte vectors the two architectures are
> equivalently supported. For all the applications, I would just say "vector" or
> "SIMD".

Okay, I kept "vector".  I don't think moving them into performance makes
sense because there I don't think this would impact user behavior or
choice, and it can't be controlled.

> And here maybe /processing/parsing/.

Done.

> > Allow xid/subxid searches to use vector operations on x86-64 architectures
> (Nathan Bossart)
> 
> When moved to the performance section, it would be something like "improve
> scalability when a large number of write transactions are in progress".

Uh, again, see above, this does not impact user behavior or choices.  I
assume this is x86-64-only.

-- 
  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

[John Naylor]

Hi Bruce,

> Add support for SSE2 (Streaming SIMD Extensions 2) vector operations on
x86-64 architectures (John Naylor)

> Add support for Advanced SIMD (Single Instruction Multiple Data) (NEON)
instructions on ARM architectures (Nathan Bossart)

Nit: It's a bit odd that SIMD is spelled out in only the Arm entry, and
perhaps expanding the abbreviations can be left out.

> Allow arrays searches to use vector operations on x86-64 architectures
(John Naylor)

We can leave out the architecture here (see below). Typo: "array searches"

All the above seem appropriate for the "source code" section, but the
following entries might be better in the "performance" section:

> Allow ASCII string detection to use vector operations on x86-64
architectures (John Naylor)
> Allow JSON string processing to use vector operations on x86-64
architectures (John Naylor)
>
> ARM?

Arm as well. For anything using 16-byte vectors the two architectures are
equivalently supported. For all the applications, I would just say "vector"
or "SIMD".

And here maybe /processing/parsing/.

> Allow xid/subxid searches to use vector operations on x86-64
architectures (Nathan Bossart)

When moved to the performance section, it would be something like "improve
scalability when a large number of write transactions are in progress".

-- 
John Naylor
EDB: http://www.enterprisedb.com

Re: Add missing includes

[Tom Lane]

Michael Paquier  writes:
> The standalone compile policy is documented in
> src/tools/pginclude/README, AFAIK, so close enough :p

Hmm, that needs an update --- the scripts are slightly smarter
now than that text gives them credit for.  I'll see to it after
the release freeze lifts.

regards, tom lane

Re: Add missing includes

[Michael Paquier]

On Mon, May 22, 2023 at 11:34:14AM -0400, Tom Lane wrote:
> There is such a policy, but not with respect to those particular
> headers.  You might find src/tools/pginclude/headerscheck and
> src/tools/pginclude/cpluspluscheck to be interesting reading,
> as those are the tests we run to verify this policy.

The standalone compile policy is documented in
src/tools/pginclude/README, AFAIK, so close enough :p
--
Michael


signature.asc
Description: PGP signature

Re: createuser --memeber and PG 16

[Michael Paquier]

On Mon, May 22, 2023 at 05:11:14AM -0700, Nathan Bossart wrote:
> On Mon, May 22, 2023 at 08:42:28AM +0200, Peter Eisentraut wrote:
> > On 21.05.23 19:07, Nathan Bossart wrote:
> >> How do folks feel about keeping --role undocumented?  Should we give it a
> >> mention in the docs for --member-of?
> > 
> > We made a point in this release to document deprecated options consistently.
> > See commit 2f80c95740.
> 
> Alright.  Does the attached patch suffice?

Seeing the precedent with --no-blobs and --blobs, yes, that should be
enough.  You may want to wait until beta1 is stamped to apply
something, though, as the period between the stamp and the tag is used
to check the state of the tarballs to-be-released.
--
Michael


signature.asc
Description: PGP signature

Re: RFI: Extending the TOAST Pointer

[Jacob Champion]

On Mon, May 22, 2023 at 6:47 AM Aleksander Alekseev
 wrote:
> Last but not least I remember somebody on the mailing list suggested
> adding ZSTD compression support for TOAST, besides LZ4. Assuming I
> didn't dream it, the proposal was rejected due to the limited amount
> of free bits in ToastCompressionId. It was argued that two possible
> combinations that are left should be treated with care and ZSTD will
> not bring enough value to the users compared to LZ4.

This thread, I think:

https://www.postgresql.org/message-id/flat/YoMiNmkztrslDbNS%40paquier.xyz

--Jacob

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

[Daniel Verite]

Kirk Wolak wrote:

> We do NOT do "CSV", we mimic pg_dump.

pg_dump uses the text format (as opposed to csv), where
\. on a line by itself cannot appear in the data, so there's
no problem. The problem is limited to the csv format.


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

Re: PG 16 draft release notes ready

[Robert Haas]

On Sun, May 21, 2023 at 3:05 PM Jonathan S. Katz  wrote:
> * Support for regular expressions for matching usernames and databases
> names in `pg_hba.conf`, and user names in `pg_ident.conf`

I suggest that this is not a major feature.

Perhaps the work that I did to improve CREATEROLE could be considered
for inclusion in the major features list. In previous releases,
someone with CREATEROLE can hack the PG OS account. Now they can't. In
previous releases, someone with CREATEROLE can manage all
non-superuser roles, but now they can manage the roles they create (or
ones they are given explicit authority to manage). You can even
control whether or not such users automatically inherit the privileges
of roles they create, as superusers inherit all privileges. There is
certainly some argument that this is not a sufficiently significant
set of changes to justify a major feature mention, and even if it is,
it's not clear to me exactly how it would be best worded. And yet I
feel like it's very likely that if we look back on this release in 3
years, those changes will have had a significant impact on many
PostgreSQL deployments, above all in the cloud, whereas I think it
likely that the ability to have regular expressions in pg_hba.conf and
pg_ident.conf will have had very little effect by comparison.

Of course, there is always a possibility that I'm over-estimating the
impact of my own work.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Order changes in PG16 since ICU introduction

[Daniel Verite]

Jeff Davis wrote:

> If we special case locale=C, but do nothing for locale=fr_FR, then I'm
> not sure we've solved the problem. Andrew Gierth raised the issue here,
> which he called "maximally confusing":
> 
> https://postgr.es/m/874jp9f5jo@news-spur.riddles.org.uk
> 
> That's why I feel that we need to make locale apply to whatever the
> provider is, not just when it happens to be C.

While I agree that the LOCALE option in CREATE DATABASE is
counter-intuitive, I find it questionable that blending ICU
and libc locales into it helps that much with the user experience.

Trying the lastest v6-* patches applied on top of 722541ead1
(before the pgindent run), here are a few examples when I
don't think it goes well.

The OS is Ubuntu 22.04 (glibc 2.35, ICU 70.1)

initdb:

  Using default ICU locale "fr".
  Using language tag "fr" for ICU locale "fr".
  The database cluster will be initialized with this locale configuration:
provider:icu
ICU locale:  fr
LC_COLLATE:  fr_FR.UTF-8
LC_CTYPE:fr_FR.UTF-8
LC_MESSAGES: fr_FR.UTF-8
LC_MONETARY: fr_FR.UTF-8
LC_NUMERIC:  fr_FR.UTF-8
LC_TIME: fr_FR.UTF-8
  The default database encoding has accordingly been set to "UTF8".


#1

postgres=# create database test1 locale='fr_FR.UTF-8';
NOTICE:  using standard form "fr-FR" for ICU locale "fr_FR.UTF-8"
ERROR:  new ICU locale (fr-FR) is incompatible with the ICU locale of the
template database (fr)
HINT:  Use the same ICU locale as in the template database, or use template0
as template.


That looks like a fairly generic case that doesn't work seamlessly.


#2

postgres=# create database test2 locale='C.UTF-8' template='template0';
NOTICE:  using standard form "en-US-u-va-posix" for ICU locale "C.UTF-8"
CREATE DATABASE


en-US-u-va-posix does not sort like C.UTF-8 in glibc 2.35, so
this interpretation is arguably not what a user would expect.

I would expect the ICU warning or error (icu_validation_level) to kick
in instead of that transliteration.


#3

$ grep french /etc/locale.alias
french  fr_FR.ISO-8859-1

postgres=# create database test3 locale='french' template='template0'
encoding='LATIN1';
WARNING:  ICU locale "french" has unknown language "french"
HINT:  To disable ICU locale validation, set parameter icu_validation_level
to DISABLED.
CREATE DATABASE


In practice we're probably getting the "und" ICU locale whereas "fr" would
be appropriate.


I assume that we would find more cases like that if testing on many
operating systems.


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

Re: benchmark results comparing versions 15.2 and 16

[MARK CALLAGHAN]

I ran sysbench on Postgres 15.2, 15.3 and 16 prebeta at git sha 1c006c0
(built on May19).
The workload was in-memory on a small server (8 cores, 16G RAM) and the
workload had 1 connection (no concurrency).
For some details on past benchmarks like this see:
http://smalldatum.blogspot.com/2023/03/searching-for-performance-regressions.html

My focus is on changes >= 10%, so a value <= 0.90 or >= 1.10.
I used 3 builds of Postgres that I call def, o2_nofp, o3_nofp and ran the
benchmark once per build. The results for each build are similar
and I only share the o2_nofp results there.

Good news, that I have not fully explained ...

One of the microbenchmarks gets ~1.5X more transactions/second (TPS) in PG
16 prebeta vs 15.2 and 15.3 for a read-only transaction that does:
* 2 select statements that scans 10k rows from an index (these are Q1 and
Q3 below and are slower in PG 16)
* 2 select statements that scans 10k rows from an index and does
aggregation (these are Q2 and Q4 below are are a lot faster in PG 16)

The speedup for Q2 and Q4 is larger than the slowdown for Q1/Q3 so TPS is
~1.5X more for PG 16.
Query plans don't appear to have changed. I assume some code got slower and
some got faster for the same plan.

The microbenchmarks are read-only_range=1 and read-only.pre_range=1
show.
Each of these microbenchmarks run a read-only transaction with 4 SQL
statements. The statements are here:
https://github.com/mdcallag/mytools/blob/master/bench/sysbench.lua/lua/oltp_common.lua#LL301C1-L312C21

read-only.pre_range runs before a large number of writes, so the b-tree
will be more read-friendly.
read-only.range runs after a large number of writes.

The =1 means that each SQL statement processes 1 rows. Note that
the microbenchmarks are also run for =100 and =10
and for those the perf with PG16 is similar to 15.x rather than ~1.5X
faster.

---

This table shows throughput relative to the base case. The base case is PG
15.2 with the o2_nofp build.
Throughput relative < 1.0 means perf regressed, > 1.0 means perf improved

col-1 : PG 15.3 with the o2_nofp build
col-2 : PG 16 prebeta build on May 19 at git sha 1c006c0 with the o2_nofp
build

col-1   col-2
0.991.03hot-points_range=100
1.021.05point-query.pre_range=100
1.061.10point-query_range=100
0.971.01points-covered-pk.pre_range=100
0.981.02points-covered-pk_range=100
0.981.01points-covered-si.pre_range=100
1.001.00points-covered-si_range=100
1.001.01points-notcovered-pk.pre_range=100
1.001.01points-notcovered-pk_range=100
1.011.03points-notcovered-si.pre_range=100
1.011.01points-notcovered-si_range=100
1.000.99random-points.pre_range=1000
1.001.02random-points.pre_range=100
1.011.01random-points.pre_range=10
1.011.00random-points_range=1000
1.011.01random-points_range=100
1.021.01random-points_range=10
1.001.00range-covered-pk.pre_range=100
1.001.00range-covered-pk_range=100
1.000.99range-covered-si.pre_range=100
1.000.99range-covered-si_range=100
1.031.01range-notcovered-pk.pre_range=100
1.021.00range-notcovered-pk_range=100
1.011.01range-notcovered-si.pre_range=100
1.011.01range-notcovered-si_range=100
1.041.54read-only.pre_range=1   <<
1.001.00read-only.pre_range=100
1.011.01read-only.pre_range=10
1.031.45read-only_range=1<<
1.011.01read-only_range=100
1.041.00read-only_range=10
1.000.99scan_range=100
1.001.02delete_range=100
1.011.02insert_range=100
1.011.00read-write_range=100
1.010.98read-write_range=10
1.011.01update-index_range=100
1.001.00update-inlist_range=100
1.021.02update-nonindex_range=100
1.031.03update-one_range=100
1.021.02update-zipf_range=100
1.031.03write-only_range=1

---

The read-only transaction has 4 SQL statements. I ran explain analyze for
each of them assuming the range scan fetches 10k rows and then 100k rows.
The 10k result is similar to what was done above, then I added the 100k
result to see if the perf difference changes with more rows.

In each case there are two "Execution Time" entries. The top one is from PG
15.2 and the bottom from PG 16 prebeta

Summary:
* Queries that do a sort show the largest improvement in PG 16 (Q2, Q4)
* Queries that don't do a sort are slower in PG 16 (Q1, Q3)

Q1.10k: explain analyze SELECT c FROM sbtest1 WHERE id BETWEEN 1000 AND
1001;

 Execution Time: 4.222 ms

 Execution Time: 6.243 ms


Q1.100k: explain analyze SELECT c FROM sbtest1 WHERE id BETWEEN 1000
AND 1010;

 Execution Time: 36.508 ms

 Execution Time: 49.344 ms


Q2.10k: explain analyze SELECT c FROM sbtest1 WHERE id BETWEEN 1000 AND
1001 order by c;

 Execution Time: 38.224 ms

 Execution Time: 

Re: PostgreSQL 16 Beta 1 release announcement draft

[Erik Rijkers]

Op 5/21/23 om 19:07 schreef Jonathan S. Katz:

On 5/19/23 12:17 AM, Jonathan S. Katz wrote:

Hi,

Attached is a draft of the release announcement for PostgreSQL 16 Beta 
Please provide feedback no later than May 24, 0:00 AoE. This will give 
Thanks everyone for your feedback. Here is the updated text that 


'substransaction'  should be
'subtransaction'

'use thousands separators'  perhaps is better:
'use underscore as digit-separator, as in `5_432` and `1_00_000`'

'instrcut'  should be
'instruct'


Erik

Re: Order changes in PG16 since ICU introduction

[Jeff Davis]

On Mon, 2023-05-22 at 14:27 +0200, Peter Eisentraut wrote:
> The rules are for setting whatever sort order you like.  Maybe you
> want 
> to sort + before - or whatever.  It's like, if you don't like it,
> build 
> your own.

A build-your-own feature is fine, but it's not completely zero cost.

There some risk that rules specified for ICU version X fail to load for
ICU version Y. If that happens to your database default collation, you
are in big trouble. The risk of failing to load a language tag in a
later version, especially one returned by uloc_toLanguageTag() in
strict mode, is much lower. We can reduce the risk by allowing rules
only for CREATE COLLATION (not  CREATE DATABASE), and see what users do
with it first, and consider adding it to CREATE DATABASE later.

We can also try to explain in the docs that it's a build-it-yourself
kind of feature (use it if you see a purpose, otherwise ignore it),
though I'm not sure quite how we should word it.

And I'm skeptical that we don't have a single plausible end-to-end user
story. I just can't think of any reason someone would need something
like this, given how flexible the collation settings in the language
tags are. The best case I can think of is if someone is trying to make
an ICU collation that matches some non-ICU collation in another system,
which sounds hard; but perhaps it's reasonable to do in cases where it
just needs to work well-enough in some limited case.

Also, do we have an answer as to why specifying the rules as '' is not
the same as not specifying any rules[1]?

[1]
https://www.postgresql.org/message-id/36a6e89689716c2ca1fae8adc8e84601a041121c.ca...@j-davis.com

> The co settings are just everything else. 
> They are not parametric, they are just some other sort order that 
> someone spelled out explicitly.

This sounds like another case where we can't really tell the user why
they would want to use a specific "co" setting; they should only use it
if they already know they want it. Is there some way we can word that
in the documentation so that people don't misuse them?

For instance, one of them is called "emoji". I'm sure a lot of
applications use emoji (or at least might encounter them), should they
always use co-emoji, or would some people who are using emoji not want
it? Can it be combined with "ks" or other "k*" settings?

What I'm trying to avoid is users seeing something in the documentation
and using it without it really being a good fit for their problem. Then
they see something unexpected, and need to rebuild all of their indexes
or something.

> > * I don't understand what "kc" means if "ks" is not set to
> > "level1".
> 
> There is an example here: 
> https://peter.eisentraut.org/blog/2023/05/16/overview-of-icu-collation-settings#colcaselevel

Interesting, thank you.

Regards,
Jeff Davis

Re: PG 16 draft release notes ready

[Bruce Momjian]

On Mon, May 22, 2023 at 11:03:31AM -0700, Andres Freund wrote:
> On 2023-05-21 22:52:09 -0400, Bruce Momjian wrote:
> > Do average users know heap and index files are both relations?  That
> > seems too abstract so I spelled out heap and index pages.
> 
> I don't know about average users - but I think users that read the release
> notes do know.
> 
> I am a bit on the fence about "addition" vs "extending" - for me it's not
> clear what "adding pages" really means, but I might be too deep into this.

I am worried "extending" and "extensions" might be too close a wording
since we often mention extensions.  I tried "increase the file eize" but
that seemed wordy.  Ideas?

Personally, while I consider heap and indexes to be both relations at
the SQL level, at the file system level I tend to think of them as
different, but perhaps that is just me.

-- 
  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 Mon, May 22, 2023 at 10:59:36AM -0700, Andres Freund wrote:
> On 2023-05-21 22:46:58 -0400, Bruce Momjian wrote:
> > > For the above two items, I mention items that would change user
> > > like new features or changes that are significant enough that they would
> > > change user behavior.  For example, if a new join method increases
> > > performance by 5x, that could change user behavior.  Based on the quoted
> > > numbers above, I didn't think "hash now faster" would be appropriate to
> > > mention.  Right?
> 
> I continue, as in past releases, to think that this is a bad policy. For
> existing workloads performance improvements are commonly a more convincing
> reason to upgrade than new features - they allow users to scale the workload
> further, without needing application changes.
> 
> Of course there are performance improvement that are too miniscule to be worth
> mentioning, but it's not a common case.
> 
> And here it's not just performance, but also memory usage, including steady
> state memory usage.

Understood.  I continue to need help determining which items to include.
Can you suggest some text?  This?

Improve efficiency of memory usage to allow for better scaling

-- 
  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

[Andres Freund]

Hi,

On 2023-05-21 22:52:09 -0400, Bruce Momjian wrote:
> On Sun, May 21, 2023 at 10:13:41AM -0700, Andres Freund wrote:
> > Hi,
> > 
> > Thanks for the release notes!
> > 
> > > 
> > > 
> > > 
> > > 
> > > Allow more efficient addition of multiple heap and index pages (Andres 
> > > Freund)
> > > 
> > > 
> > 
> > While the case of extending by multiple pages improved the most, even
> > extending by a single page at a time got a good bit more scalable. Maybe 
> > just
> > "Improve efficiency of extending relations"?
> 
> Do average users know heap and index files are both relations?  That
> seems too abstract so I spelled out heap and index pages.

I don't know about average users - but I think users that read the release
notes do know.

I am a bit on the fence about "addition" vs "extending" - for me it's not
clear what "adding pages" really means, but I might be too deep into this.

Greetings,

Andres Freund

Re: PG 16 draft release notes ready

[Bruce Momjian]

I have added the major features to the release notes with the attached
patch.

---

On Sun, May 21, 2023 at 07:53:38PM -0400, Jonathan Katz wrote:
> On 5/21/23 3:04 PM, Jonathan S. Katz wrote:
> > On 5/18/23 4:49 PM, Bruce Momjian wrote:
> > > I have completed the first draft of the PG 16 release notes.
> > 
> > One thing that we could attempt for this beta is to include a
> > prospective list of "major features + enhancements." Of course it can
> > change before the GA, but it'll give readers some idea of things to
> > test.
> > 
> > I'd propose the following (in no particular order):
> > 
> > * General performance improvements for read-heavy workloads (looking for
> > clarification for that in[1])
> 
> Per [1] this sounds like it should be:
> 
> * Optimization to reduce overall memory usage, including general performance
> improvements.
> 
> We can get more specific for the GA.
> 
> Thanks,
> 
> Jonathan
> 
> [1] 
> https://www.postgresql.org/message-id/5749E807-A5B7-4CC7-8282-84F6F0D4D1D0%40anarazel.de
> 




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

  Only you can decide what is important to you.
commit 60751aa503
Author: Bruce Momjian 
Date:   Mon May 22 13:58:24 2023 -0400

doc: PG 16 relnotes, add major features list

Reported-by: Jonathan Katz

Discussion: https://postgr.es/m/2fd2cc0e-df39-3e77-8fcf-35aad5796...@postgresql.org

diff --git a/doc/src/sgml/release-16.sgml b/doc/src/sgml/release-16.sgml
index 893cd8ddb0..3d96bd6e6d 100644
--- a/doc/src/sgml/release-16.sgml
+++ b/doc/src/sgml/release-16.sgml
@@ -18,7 +18,49 @@

 

-ADD HERE
+
+
+ 
+  Allow parallel execution of queries with OUTER and FULL joins
+ 
+
+
+
+ 
+  Allow logical replication from standbys servers
+ 
+
+
+
+ 
+  Allow logical replication subscribers to apply large transactions in parallel
+ 
+
+
+
+ 
+  Allow monitoring of I/O statistics using the new pg_stat_io view
+ 
+
+
+
+ 
+  Add SQL/JSON constructors and identity functions
+ 
+
+
+
+ 
+  Improve performance of vacuum freezing
+ 
+
+
+
+ 
+  Add support for regular expression matching of user and database names in pg_hba.conf, and user names in pg_ident.conf
+ 
+
+

 

@@ -280,7 +322,7 @@ Author: Thomas Munro 
 
 
 
-Allow full and outer joins to be performed in parallel (Melanie Plageman, Thomas Munro)
+Allow outer and full joins to be performed in parallel (Melanie Plageman, Thomas Munro)
 
 
 

Re: Avoiding another needless ERROR during nbtree page deletion

[Peter Geoghegan]

On Mon, May 22, 2023 at 9:22 AM Peter Geoghegan  wrote:
> > This comment notes that this is similar to what we did with the left
> > sibling, but there isn't really any mention at the left sibling code
> > about avoiding hard ERRORs. Feels a bit backwards. Maybe move the
> > comment about avoiding the hard ERROR to where the left sibling is
> > handled. Or explain it in the function comment and just have short
> > "shouldn't happen, but avoid hard ERROR if the index is corrupt" comment
> > here.
>
> Good point. Will do it that way.

Attached is v2, which does it that way. It also adjusts the approach
taken to release locks and pins when the left sibling validation check
fails. This makes it simpler and more consistent with surrounding
code. I might not include this change in the backpatch.

Not including a revised amcheck patch here, since I'm not exactly sure
what to do with your feedback on that one just yet.

-- 
Peter Geoghegan


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

Re: PG 16 draft release notes ready

[Andres Freund]

Hi,

On 2023-05-21 22:46:58 -0400, Bruce Momjian wrote:
> > Looking through the release notes, I didn't see an entry for
> >
> > commit c6e0fe1f2a08505544c410f613839664eea9eb21
> > Author: David Rowley 
> > Date:   2022-08-29 17:15:00 +1200
> >
> > Improve performance of and reduce overheads of memory management
> >
> > even though I think that's one of the more impactful improvements. What was
> > the reason for leaving that out?
>
> If you read my previous email:
>
> > For the above two items, I mention items that would change user
> > like new features or changes that are significant enough that they would
> > change user behavior.  For example, if a new join method increases
> > performance by 5x, that could change user behavior.  Based on the quoted
> > numbers above, I didn't think "hash now faster" would be appropriate to
> > mention.  Right?

I continue, as in past releases, to think that this is a bad policy. For
existing workloads performance improvements are commonly a more convincing
reason to upgrade than new features - they allow users to scale the workload
further, without needing application changes.

Of course there are performance improvement that are too miniscule to be worth
mentioning, but it's not a common case.

And here it's not just performance, but also memory usage, including steady
state memory usage.


> I can see this item as a big win, but I don't know how to describe it in a way
> that is helpful for the user to know.

In doubt the subject of the commit would just work IMO.

Greetings,

Andres Freund

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

[Andres Freund]

Hi,

On 2023-05-22 12:15:07 +, Zhijie Hou (Fujitsu) wrote:
> About "a backend doing logical decoding", do you mean the case when a user
> start a backend and invoke pg_logical_slot_get_changes() to do the logical
> decoding ? If so, it seems the logical decoding in a backend won't be waked up
> by startup process because the backend won't be registered as a walsender so
> the backend won't be found in WalSndWakeup().

I meant logical decoding happening inside a walsender instance.


> Or do you mean the deadlock between the real logical walsender and startup
> process ? (I might miss something) I think the logical decoding doesn't lock
> the target user relation when decoding because it normally can get the needed
> information from WAL.

It does lock catalog tables briefly. There's no guarantee that such locks are
released immediately. I forgot the details, but IIRC there's some outfuncs
(enum?) that intentionally delay releasing locks till transaction commit.

Greetings,

Andres Freund

Re: Order changes in PG16 since ICU introduction

[Jeff Davis]

On Thu, 2023-05-11 at 13:07 +0200, Peter Eisentraut wrote:
> Here is my proposed patch for this.

The commit message makes it sound like lc_collate/ctype are completely
obsolete, and I don't think that's quite right: they still represent
the server environment, which does still matter in some cases.

I'd just say that they are too confusing (likely to be misused), and
becoming obsolete (or less relevant), or something along those lines.

Otherwise, this is fine with me. I didn't do a detailed review because
it's just mechanical.

Regards,
Jeff Davis

Re: Make pgbench exit on SIGINT more reliably

[Tristan Partin]

Here is a v2 that handles the Windows case that I seemingly missed in my
first readthrough of this code.

-- 
Tristan Partin
Neon (https://neon.tech)
From 3cff6aec8f154eb8a47524efc15f16a6b7e95f37 Mon Sep 17 00:00:00 2001
From: Tristan Partin 
Date: Mon, 22 May 2023 08:07:37 -0500
Subject: [PATCH postgres v2 1/2] Move exit code definitions to fe_utils

This makes sharing exit code definitions much simpler.
---
 src/bin/psql/common.c |  3 ++-
 src/bin/psql/mainloop.c   |  1 +
 src/bin/psql/settings.h   | 13 -
 src/bin/psql/startup.c|  1 +
 src/include/fe_utils/exit_codes.h | 24 
 5 files changed, 28 insertions(+), 14 deletions(-)
 create mode 100644 src/include/fe_utils/exit_codes.h

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index c0e6e8e6ed..a36e188944 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -25,6 +25,7 @@
 #include "copy.h"
 #include "crosstabview.h"
 #include "fe_utils/cancel.h"
+#include "fe_utils/exit_codes.h"
 #include "fe_utils/mbprint.h"
 #include "fe_utils/string_utils.h"
 #include "portability/instr_time.h"
@@ -273,7 +274,7 @@ psql_cancel_callback(void)
 void
 psql_setup_cancel_handler(void)
 {
-	setup_cancel_handler(psql_cancel_callback);
+	setup_cancel_handler(psql_cancel_callback, NULL);
 }
 
 
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index 692c6db34c..98df0e0a97 100644
--- a/src/bin/psql/mainloop.c
+++ b/src/bin/psql/mainloop.c
@@ -10,6 +10,7 @@
 #include "command.h"
 #include "common.h"
 #include "common/logging.h"
+#include "fe_utils/exit_codes.h"
 #include "input.h"
 #include "mainloop.h"
 #include "mb/pg_wchar.h"
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 73d4b393bc..506d6db0a4 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -157,17 +157,4 @@ typedef struct _psqlSettings
 
 extern PsqlSettings pset;
 
-
-#ifndef EXIT_SUCCESS
-#define EXIT_SUCCESS 0
-#endif
-
-#ifndef EXIT_FAILURE
-#define EXIT_FAILURE 1
-#endif
-
-#define EXIT_BADCONN 2
-
-#define EXIT_USER 3
-
 #endif
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 5a28b6f713..877408f65b 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -19,6 +19,7 @@
 #include "common/logging.h"
 #include "common/string.h"
 #include "describe.h"
+#include "fe_utils/exit_codes.h"
 #include "fe_utils/print.h"
 #include "getopt_long.h"
 #include "help.h"
diff --git a/src/include/fe_utils/exit_codes.h b/src/include/fe_utils/exit_codes.h
new file mode 100644
index 00..d7136f8b50
--- /dev/null
+++ b/src/include/fe_utils/exit_codes.h
@@ -0,0 +1,24 @@
+/*
+ * psql - the PostgreSQL interactive terminal
+ *
+ * Copyright (c) 2000-2023, PostgreSQL Global Development Group
+ *
+ * src/include/fe_utils/exit_codes.h
+ */
+
+#ifndef EXIT_CODES_H
+#define EXIT_CODES_H
+
+#ifndef EXIT_SUCCESS
+#define EXIT_SUCCESS 0
+#endif
+
+#ifndef EXIT_FAILURE
+#define EXIT_FAILURE 1
+#endif
+
+#define EXIT_BADCONN 2
+
+#define EXIT_USER 3
+
+#endif
-- 
-- 
Tristan Partin
Neon (https://neon.tech)

From b90da559dc27d671095ebafa9c805f1562029508 Mon Sep 17 00:00:00 2001
From: Tristan Partin 
Date: Mon, 22 May 2023 08:19:27 -0500
Subject: [PATCH postgres v2 2/2] Add a post-PQcancel hook to
 setup_cancel_handler

This is a follow-up to 1d468b9ad81b9139b4a0b16b416c3597925af4b0. This
patch allowed pgbench to cancel server-side operations, but kept pgbench
from exiting until the only CancelRequested check was evaluated. In
addition, pgbench would not exit with a non-zero exit value given a
SIGINT.
---
 src/bin/pg_amcheck/pg_amcheck.c |  2 +-
 src/bin/pgbench/pgbench.c   | 17 +
 src/bin/scripts/clusterdb.c |  2 +-
 src/bin/scripts/reindexdb.c |  2 +-
 src/bin/scripts/vacuumdb.c  |  2 +-
 src/fe_utils/cancel.c   | 34 +++--
 src/include/fe_utils/cancel.h   |  5 -
 7 files changed, 45 insertions(+), 19 deletions(-)

diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 68f8180c19..f3f31cde0f 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ b/src/bin/pg_amcheck/pg_amcheck.c
@@ -470,7 +470,7 @@ main(int argc, char *argv[])
 	cparams.dbname = NULL;
 	cparams.override_dbname = NULL;
 
-	setup_cancel_handler(NULL);
+	setup_cancel_handler(NULL, NULL);
 
 	/* choose the database for our initial connection */
 	if (opts.alldb)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 70ed034e70..b8568f47de 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -39,6 +39,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 		/* for getrlimit */
 
@@ -60,6 +61,7 @@
 #include "common/username.h"
 #include "fe_utils/cancel.h"
 #include "fe_utils/conditional.h"
+#include "fe_utils/exit_codes.h"
 #include "fe_utils/option_utils.h"
 #include "fe_utils/string_utils.h"
 #include "getopt_long.h"
@@ -4944,9 +4946,6 @@ 

Handle SIGTERM in fe_utils/cancel.c

[Tristan Partin]

Hello,

This is a way that would solve bug #17698[1]. It just reuses the same
handler as SIGINT (with a function rename).

This patch works best if it is combined with my previous submission[2].
I can rebase that submission if and when this patch is pulled in.

[1]: 
https://www.postgresql.org/message-id/17698-58a6ab8caec496b0%40postgresql.org
[2]: https://www.postgresql.org/message-id/CSSWBAX56CVY.291H6ZNNHK7EO%40c3po

-- 
Tristan Partin
Neon (https://neon.tech)
From 42db43794c38341a088570e0033a945cc077a350 Mon Sep 17 00:00:00 2001
From: Tristan Partin 
Date: Mon, 22 May 2023 12:12:06 -0500
Subject: [PATCH postgres v1] Handle SIGTERM in fe_utils/cancel.c

This is the same handling as SIGINT.

This issue was originally reported in
https://www.postgresql.org/message-id/17698-58a6ab8caec496b0%40postgresql.org.
---
 src/fe_utils/cancel.c | 7 ---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/src/fe_utils/cancel.c b/src/fe_utils/cancel.c
index 10c0cd4554..b13003d405 100644
--- a/src/fe_utils/cancel.c
+++ b/src/fe_utils/cancel.c
@@ -150,7 +150,7 @@ ResetCancelConn(void)
  * is set.
  */
 static void
-handle_sigint(SIGNAL_ARGS)
+handle_signal(SIGNAL_ARGS)
 {
 	int			save_errno = errno;
 	char		errbuf[256];
@@ -180,7 +180,7 @@ handle_sigint(SIGNAL_ARGS)
 /*
  * setup_cancel_handler
  *
- * Register query cancellation callback for SIGINT.
+ * Register query cancellation callback for SIGINT and SIGTERM.
  */
 void
 setup_cancel_handler(void (*query_cancel_callback) (void))
@@ -189,7 +189,8 @@ setup_cancel_handler(void (*query_cancel_callback) (void))
 	cancel_sent_msg = _("Cancel request sent\n");
 	cancel_not_sent_msg = _("Could not send cancel request: ");
 
-	pqsignal(SIGINT, handle_sigint);
+	pqsignal(SIGINT, handle_signal);
+	pqsignal(SIGTERM, handle_signal);
 }
 
 #else			/* WIN32 */
-- 
-- 
Tristan Partin
Neon (https://neon.tech)

Re: RFI: Extending the TOAST Pointer

[Matthias van de Meent]

On Mon, 22 May 2023 at 18:13, Nikita Malakhov  wrote:
>
> Hi,

Could you  please not top-post.

> Aleksander, I'm interested in extending TOAST pointer in various ways.
> 64-bit TOAST value ID allows to resolve very complex issue for production
> systems with large tables and heavy update rate.

Cool. I agree that this would be nice, though it's quite unlikely
we'll ever be able to use much more than 32 bits concurrently with the
current 32-bit block IDs. But indeed, it is a good way of reducing
time spent searching for unused toast IDs.

> I agree with Matthias that there should not be processing of TOAST pointer
> internals outside TOAST macros. Currently, TOASTed value is distinguished
> as VARATT_IS_EXTERNAL_ONDISK, and it should stay this way. Adding
> compression requires another implementation (extension) of
> VARATT_EXTERNAL because current supports only 2 compression methods -
> it has only 1 bit responsible for compression method, and there is a safe
> way to do so, without affecting default TOAST mechanics - we must keep
> it this way for compatibility issues and not to break DB upgrade.
>
> Also, I must remind that we should not forget about field alignment inside
> the TOAST pointer.

What field alignment inside the TOAST pointers?
Current TOAST pointers are not aligned: the varatt_external struct is
copied into and from the va_data section of varattrib_1b_e when we
need to access the data; so as far as I know this struct has no
alignment to speak of.

> As it was already mentioned, it seems not very reasonable trying to save
> a byte or two while we are storing out-of-line values of at least 2 kb in 
> size.

If we were talking about the data we're storing externally, sure. But
this is data we store in the original tuple, and moving that around is
relatively expensive. Reducing the aligned size of the toast pointer
can help reduce the size of the heap tuple, thus increasing the
efficiency of the primary data table.

Kind regards,

Matthias van de Meent
Neon, Inc.

Re: Order changes in PG16 since ICU introduction

[Jeff Davis]

On Thu, 2023-05-11 at 13:09 +0200, Peter Eisentraut wrote:
> There is also the deterministic flag and the icurules setting. 
> Depending on what level of detail you imagine the user needs, you
> really 
> do need to look at the whole picture, not some subset of it.

(Nit: all database default collations are deterministic.)

I agree, but I think there should be a way to see the whole picture in
one command. If nothing else, for repro cases sent to the list, it
would be nice to have a single line like:

   SELECT show_default_collation_whole_picture();

Right now it involves some back and forth, like checking
datlocprovider, then looking in the right fields and ignoring the wrong
ones.

Regards,
Jeff Davis

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

[Kirk Wolak]

On Mon, May 22, 2023 at 12:13 PM Daniel Verite 
wrote:

> Joel Jacobson wrote:
>
> > Is there a valid reason why \. is needed for COPY FROM filename?
> > It seems to me it would only be necessary for the COPY FROM STDIN case,
> > since files have a natural end-of-file and a known file size.
>
> Looking at CopyReadLineText() over at [1], I don't see a reason why
> the unquoted \. could not be handled with COPY FROM file.
> Even COPY FROM STDIN looks like it could be benefit, so that
> \copy from file csv would hopefully not choke or truncate the data.
> There's still the case when the CSV data is embedded in a psql script
> (psql is unable to know where it ends), but for that, "don't do that"
> might be a reasonable answer.
>

Don't have what looks like a pg_dump script?
We specifically create such SQL files with embedded data.  Depending on the
circumstances,
we either confirm that indexes dropped and triggers are disabled...  [Or we
create a dynamic name,
and insert it into a queue table for later processing], and then we COPY
the data, ending in
\.

We do NOT do "CSV", we mimic pg_dump.

Now, if you are talking about only impacting a fixed data file format...
Sure.  But impacting how psql
processes these \i  included files...  (that could hurt)

Re: Avoiding another needless ERROR during nbtree page deletion

[Peter Geoghegan]

On Sun, May 21, 2023 at 11:51 PM Heikki Linnakangas  wrote:
> Any idea what might cause this corruption?

Not really, no. As far as I know the specific case that was brought to
my attention (that put me on the path to writing this patch) was just
an isolated incident. The interesting detail (if any) is that it was a
relatively recent version of Postgres (13), and that there were no
other known problems. This means that there is a plausible remaining
gap in the defensive checks in nbtree VACUUM on recent versions -- we
might have expected to avoid a hard ERROR in some other way, from one
of the earlier checks, but that didn't happen on at least one
occasion.

You can find several references to the "right sibling's left-link
doesn't match:" error message by googling. Most of them are clearly
from the page split ERROR. But there are some from VACUUM, too:

https://stackoverflow.com/questions/49307292/error-in-postgresql-right-siblings-left-link-doesnt-match-block-5-links-to-8

Granted, that was from a 9.2 database -- before your 9.4 work that
made this whole area much more robust.

> This comment notes that this is similar to what we did with the left
> sibling, but there isn't really any mention at the left sibling code
> about avoiding hard ERRORs. Feels a bit backwards. Maybe move the
> comment about avoiding the hard ERROR to where the left sibling is
> handled. Or explain it in the function comment and just have short
> "shouldn't happen, but avoid hard ERROR if the index is corrupt" comment
> here.

Good point. Will do it that way.

> > 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.

> You could check that the left sibling is indeed a half-dead page.

It's very hard to see, but...I think that we do. Sort of. Since
bt_recheck_sibling_links() is prepared to check that the left
sibling's right link points back to the target page.

One problem with that is that it only happens in the AccessShareLock
case, whereas we're concerned with fixing an issue in the ShareLock
case. Another problem is that it's awkward and complicated to explain.
It's not obvious that it's worth trying to explain all this and/or
making sure that it happens in the ShareLock case, so that we have
everything covered. I'm unsure.

> ERRCODE_NO_DATA doesn't look right. Let's just leave out the errcode.

Agreed.

-- 
Peter Geoghegan

Re: RFI: Extending the TOAST Pointer

[Nikita Malakhov]

Hi,

Aleksander, I'm interested in extending TOAST pointer in various ways.
64-bit TOAST value ID allows to resolve very complex issue for production
systems with large tables and heavy update rate.

I agree with Matthias that there should not be processing of TOAST pointer
internals outside TOAST macros. Currently, TOASTed value is distinguished
as VARATT_IS_EXTERNAL_ONDISK, and it should stay this way. Adding
compression requires another implementation (extension) of
VARATT_EXTERNAL because current supports only 2 compression methods -
it has only 1 bit responsible for compression method, and there is a safe
way to do so, without affecting default TOAST mechanics - we must keep
it this way for compatibility issues and not to break DB upgrade.

Also, I must remind that we should not forget about field alignment inside
the TOAST pointer.

As it was already mentioned, it seems not very reasonable trying to save
a byte or two while we are storing out-of-line values of at least 2 kb in
size.

On Mon, May 22, 2023 at 4:47 PM Aleksander Alekseev <
aleksan...@timescale.com> wrote:

> Hi,
>
> > I see your point, but I do think we should also think about why we do
> > the change.
>
> Personally at the moment I care only about implementing compression
> dictionaries on top of this, as is discussed in the corresponding
> thread [1]. I'm going to need new fields in the TOAST pointer's
> including (but not necessarily limited to) dictionary id.
>
> As I understand, Nikita is interested in implementing 64-bit TOAST
> pointers [2]. I must admit I didn't follow that thread too closely but
> I can imagine the needs are similar.
>
> Last but not least I remember somebody on the mailing list suggested
> adding ZSTD compression support for TOAST, besides LZ4. Assuming I
> didn't dream it, the proposal was rejected due to the limited amount
> of free bits in ToastCompressionId. It was argued that two possible
> combinations that are left should be treated with care and ZSTD will
> not bring enough value to the users compared to LZ4.
>
> These are 3 recent cases I could recall. This being said I think our
> solution should be generic enough to cover possible future cases
> and/or cases unknown to us yet.
>
> [1]:
> https://postgr.es/m/CAJ7c6TM7%2BsTvwREeL74Y5U91%2B5ymNobRbOmnDRfdTonq9trZyQ%40mail.gmail.com
> [2]: https://commitfest.postgresql.org/43/4296/
>
> --
> Best regards,
> Aleksander Alekseev
>


-- 
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/

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

[Daniel Verite]

Joel Jacobson wrote:

> Is there a valid reason why \. is needed for COPY FROM filename?
> It seems to me it would only be necessary for the COPY FROM STDIN case,
> since files have a natural end-of-file and a known file size.

Looking at CopyReadLineText() over at [1], I don't see a reason why
the unquoted \. could not be handled with COPY FROM file.
Even COPY FROM STDIN looks like it could be benefit, so that
\copy from file csv would hopefully not choke or truncate the data.
There's still the case when the CSV data is embedded in a psql script
(psql is unable to know where it ends), but for that, "don't do that"
might be a reasonable answer.


[1]
https://doxygen.postgresql.org/copyfromparse_8c.html#a90201f711221dd82d0c08deedd91e1b3



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

Re: Add the ability to limit the amount of memory that can be allocated to backends.

[reid . thompson]

On Mon, 2023-05-22 at 08:42 -0400, reid.thomp...@crunchydata.com wrote:
> On Wed, 2023-05-17 at 23:07 -0400, reid.thomp...@crunchydata.com wrote:
> > Thanks for the feedback.
> > 
> > I'm plannning to look at this. 
> > 
> > Is your benchmark something that I could utilize? I.E. is it a set of
> > scripts or a standard test from somewhere that I can duplicate?
> > 
> > Thanks,
> > Reid
> > 

Attach patches updated to master.
Pulled from patch 2 back to patch 1 a change that was also pertinent to patch 1.

From e6f8499e0270f2291494260bc341e8ad1411c2ae Mon Sep 17 00:00:00 2001
From: Reid Thompson 
Date: Thu, 11 Aug 2022 12:01:25 -0400
Subject: [PATCH 1/2] Add tracking of backend memory allocated

Add tracking of backend memory allocated in total and by allocation
type (aset, dsm, generation, slab) by process.

allocated_bytes tracks the current bytes of memory allocated to the
backend process. aset_allocated_bytes, dsm_allocated_bytes,
generation_allocated_bytes and slab_allocated_bytes track the
allocation by type for the backend process. They are updated for the
process as memory is malloc'd/freed.  Memory allocated to items on
the freelist is included.  Dynamic shared memory allocations are
included only in the value displayed for the backend that created
them, they are not included in the value for backends that are
attached to them to avoid double counting. DSM allocations that are
not destroyed by the creating process prior to it's exit are
considered long lived and are tracked in a global counter
global_dsm_allocated_bytes. We limit the floor of allocation
counters to zero. Created views pg_stat_global_memory_allocation and
pg_stat_memory_allocation for access to these trackers.
---
 doc/src/sgml/monitoring.sgml| 246 
 src/backend/catalog/system_views.sql|  34 +++
 src/backend/storage/ipc/dsm.c   |  11 +-
 src/backend/storage/ipc/dsm_impl.c  |  78 +++
 src/backend/storage/lmgr/proc.c |   1 +
 src/backend/utils/activity/backend_status.c | 114 +
 src/backend/utils/adt/pgstatfuncs.c |  84 +++
 src/backend/utils/init/miscinit.c   |   3 +
 src/backend/utils/mmgr/aset.c   |  17 ++
 src/backend/utils/mmgr/generation.c |  15 ++
 src/backend/utils/mmgr/slab.c   |  22 ++
 src/include/catalog/pg_proc.dat |  17 ++
 src/include/storage/proc.h  |   2 +
 src/include/utils/backend_status.h  | 144 +++-
 src/test/regress/expected/rules.out |  27 +++
 src/test/regress/expected/stats.out |  36 +++
 src/test/regress/sql/stats.sql  |  20 ++
 17 files changed, 869 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index df5242fa80..cfc221fb2e 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -5757,6 +5757,252 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i
 
  
 
+ 
+  pg_stat_memory_allocation
+
+  
+   pg_stat_memory_allocation
+  
+
+  
+   The pg_stat_memory_allocation view will have one
+   row per server process, showing information related to the current memory
+   allocation of that process in total and by allocator type. Due to the
+   dynamic nature of memory allocations the allocated bytes values may not be
+   exact but should be sufficient for the intended purposes. Dynamic shared
+   memory allocations are included only in the value displayed for the backend
+   that created them, they are not included in the value for backends that are
+   attached to them to avoid double counting.  Use
+   pg_size_pretty described in
+to make these values more easily
+   readable.
+  
+
+  
+   pg_stat_memory_allocation View
+   
+
+ 
+  
+   Column Type
+  
+  
+   Description
+  
+ 
+
+
+
+ 
+  
+   datid oid
+  
+  
+   OID of the database this backend is connected to
+  
+ 
+
+ 
+  
+   pid integer
+  
+  
+   Process ID of this backend
+  
+ 
+
+ 
+  
+   allocated_bytes bigint
+  
+ 
+  Memory currently allocated to this backend in bytes. This is the balance
+  of bytes allocated and freed by this backend. Dynamic shared memory
+  allocations are included only in the value displayed for the backend that
+  created them, they are not included in the value for backends that are
+  attached to them to avoid double counting.
+ 
+ 
+
+ 
+  
+   aset_allocated_bytes bigint
+  
+  
+   Memory currently allocated to this backend in bytes via the allocation
+   set allocator.
+  
+ 
+
+ 
+  
+   dsm_allocated_bytes bigint
+  
+  
+   Memory currently allocated to this backend in bytes via the dynamic
+   shared memory allocator. Upon process exit, dsm allocations that have
+   

Re: Naming of gss_accept_deleg

[Tom Lane]

Abhijit Menon-Sen  writes:
> Here's the diff,

Pushed, thanks.

> but the 0/1 values of settings like sslsni and
> sslcompression don't seem to be validated anywhere, unlike the string
> options in connectOptions2, so I didn't do anything for gssdelegation.

Yeah.  Perhaps it's worth adding code to validate boolean options,
but since nobody has noticed the lack of that for decades, I'm not
in a hurry to (especially not in a last-minute patch).

Also, I noticed that PGGSSDELEGATION had not been added to the lists of
environment variables to unset in pg_regress.c and Test/Utils.pm.
Dealt with that in the same commit.

regards, tom lane

Re: Add missing includes

[Tom Lane]

"Tristan Partin"  writes:
> Interesting. Thanks for the information. Thought I had seen in a
> different email thread that header files were supposed to include all
> that they needed to. Anyways, ignore the patch :).

There is such a policy, but not with respect to those particular
headers.  You might find src/tools/pginclude/headerscheck and
src/tools/pginclude/cpluspluscheck to be interesting reading,
as those are the tests we run to verify this policy.

regards, tom lane

Re: Add missing includes

[Tristan Partin]

On Mon May 22, 2023 at 10:28 AM CDT, Tom Lane wrote:
> >> Some files were missing information from the c.h header.
>
> > Actually, these omissions are intentional, and we have bespoke handling
> > for this in our own header-checking scripts (src/tools/pginclude).  I
> > imagine this is documented somewhere, but ATM I can't remember where.
> > (And if not, maybe that's something we should do.)
>
> Yeah, the general policy is that .h files should not explicitly include
> c.h (nor postgres.h nor postgres_fe.h).  Instead, .c files should include
> the appropriate one of those three files first.  This allows sharing of
> .h files more easily across frontend/backend/common environments.
>
> I'm not sure where this is documented either.

Thanks for the added context. I'll try to keep this in mind going
forward.

-- 
Tristan Partin
Neon (https://neon.tech)

Re: Add missing includes

[Tom Lane]

Alvaro Herrera  writes:
> On 2023-May-22, Tristan Partin wrote:
>> Some files were missing information from the c.h header.

> Actually, these omissions are intentional, and we have bespoke handling
> for this in our own header-checking scripts (src/tools/pginclude).  I
> imagine this is documented somewhere, but ATM I can't remember where.
> (And if not, maybe that's something we should do.)

Yeah, the general policy is that .h files should not explicitly include
c.h (nor postgres.h nor postgres_fe.h).  Instead, .c files should include
the appropriate one of those three files first.  This allows sharing of
.h files more easily across frontend/backend/common environments.

I'm not sure where this is documented either.

regards, tom lane

Re: Add missing includes

[Tristan Partin]

On Mon May 22, 2023 at 10:16 AM CDT, Alvaro Herrera wrote:
> > Some files were missing information from the c.h header.
>
> Actually, these omissions are intentional, and we have bespoke handling
> for this in our own header-checking scripts (src/tools/pginclude).  I
> imagine this is documented somewhere, but ATM I can't remember where.
> (And if not, maybe that's something we should do.)

Interesting. Thanks for the information. Thought I had seen in a
different email thread that header files were supposed to include all
that they needed to. Anyways, ignore the patch :).

-- 
Tristan Partin
Neon (https://neon.tech)

Re: Add missing includes

[Alvaro Herrera]

Hi,

On 2023-May-22, Tristan Partin wrote:

> Some files were missing information from the c.h header.

Actually, these omissions are intentional, and we have bespoke handling
for this in our own header-checking scripts (src/tools/pginclude).  I
imagine this is documented somewhere, but ATM I can't remember where.
(And if not, maybe that's something we should do.)

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Add missing includes

[Tristan Partin]

Hi,

Some files were missing information from the c.h header.

-- 
Tristan Partin
Neon (https://neon.tech)
From 3c0c32077bb9626d4ebad1452b9fc9937c99518d Mon Sep 17 00:00:00 2001
From: Tristan Partin 
Date: Mon, 22 May 2023 10:07:45 -0500
Subject: [PATCH postgres v1] Add some missing includes

Both files were missing defines/types from c.h. Issue found through
usage of clangd.
---
 src/bin/psql/settings.h   | 1 +
 src/include/fe_utils/cancel.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 73d4b393bc..f429ca6ad9 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -8,6 +8,7 @@
 #ifndef SETTINGS_H
 #define SETTINGS_H
 
+#include "c.h"
 #include "fe_utils/print.h"
 #include "variables.h"
 
diff --git a/src/include/fe_utils/cancel.h b/src/include/fe_utils/cancel.h
index 6e8b9ddfc4..733aaf7751 100644
--- a/src/include/fe_utils/cancel.h
+++ b/src/include/fe_utils/cancel.h
@@ -16,6 +16,7 @@
 
 #include 
 
+#include "c.h"
 #include "libpq-fe.h"
 
 extern PGDLLIMPORT volatile sig_atomic_t CancelRequested;
-- 
-- 
Tristan Partin
Neon (https://neon.tech)

Re: Naming of gss_accept_deleg

[Tom Lane]

Abhijit Menon-Sen  writes:
> Here's the diff, but the 0/1 values of settings like sslsni and
> sslcompression don't seem to be validated anywhere, unlike the string
> options in connectOptions2, so I didn't do anything for gssdelegation.

Thanks!  I'll set to work on this.  I assume we want to squeeze
it into beta1, so there's not much time.

regards, tom lane

Re: Naming of gss_accept_deleg

[Abhijit Menon-Sen]

At 2023-05-22 09:42:44 -0400, t...@sss.pgh.pa.us wrote:
>
> Alvaro Herrera  writes:
> > I noticed that the value that enables this feature at libpq client side
> > is 'enable'.  However, for other Boolean settings like sslsni,
> > keepalives, requiressl, sslcompression, the value that enables feature
> > is '1' -- we use strings only for "enum" type of settings.
> 
> > Also, it looks like connectOptions2() doesn't validate the string value.
> 
> Hmm, it certainly seems like this ought to accept exactly the
> same inputs as other libpq boolean settings.  I can take a look
> unless somebody else is already on it.

Here's the diff, but the 0/1 values of settings like sslsni and
sslcompression don't seem to be validated anywhere, unlike the string
options in connectOptions2, so I didn't do anything for gssdelegation.

(I've never run the Kerberos tests before, but I changed one
"gssdelegation=disable" to "gssdelegation=1" and got a test failure, so
they're probably working as expected.)

-- Abhijit
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index e38a7debc3..2225e4e0ef 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2059,9 +2059,9 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   

 Forward (delegate) GSS credentials to the server.  The default is
-disable which means credentials will not be forwarded
-to the server.  Set this to enable to have
-credentials forwarded when possible.
+0 which means credentials will not be forwarded
+to the server.  Set this to 1 to have credentials
+forwarded when possible.

   
  
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index de0e13e50d..88fd0f3d80 100644
--- a/src/interfaces/libpq/fe-auth.c
+++ b/src/interfaces/libpq/fe-auth.c
@@ -97,7 +97,7 @@ pg_GSS_continue(PGconn *conn, int payloadlen)
 	if (!pg_GSS_have_cred_cache(>gcred))
 		conn->gcred = GSS_C_NO_CREDENTIAL;
 
-	if (conn->gssdelegation && pg_strcasecmp(conn->gssdelegation, "enable") == 0)
+	if (conn->gssdelegation && conn->gssdelegation[0] == '1')
 		gss_flags |= GSS_C_DELEG_FLAG;
 
 	maj_stat = gss_init_sec_context(_stat,
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 786d22a770..a8584d2c68 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -343,8 +343,8 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
 		"GSS-library", "", 7,	/* sizeof("gssapi") == 7 */
 	offsetof(struct pg_conn, gsslib)},
 
-	{"gssdelegation", "PGGSSDELEGATION", NULL, NULL,
-		"GSS-delegation", "", 8,	/* sizeof("disable") == 8 */
+	{"gssdelegation", "PGGSSDELEGATION", "0", NULL,
+		"GSS-delegation", "", 1,
 	offsetof(struct pg_conn, gssdelegation)},
 
 	{"replication", NULL, NULL, NULL,
diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c
index c77d5cfe9f..7e373236e9 100644
--- a/src/interfaces/libpq/fe-secure-gssapi.c
+++ b/src/interfaces/libpq/fe-secure-gssapi.c
@@ -622,7 +622,7 @@ pqsecure_open_gss(PGconn *conn)
 	if (ret != STATUS_OK)
 		return PGRES_POLLING_FAILED;
 
-	if (conn->gssdelegation && pg_strcasecmp(conn->gssdelegation, "enable") == 0)
+	if (conn->gssdelegation && conn->gssdelegation[0] == '1')
 	{
 		/* Acquire credentials if possible */
 		if (conn->gcred == GSS_C_NO_CREDENTIAL)
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index f1854f9919..0045f83cbf 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -404,7 +404,7 @@ struct pg_conn
 	char	   *krbsrvname;		/* Kerberos service name */
 	char	   *gsslib;			/* What GSS library to use ("gssapi" or
  * "sspi") */
-	char	   *gssdelegation;	/* Try to delegate GSS credentials? */
+	char	   *gssdelegation;	/* Try to delegate GSS credentials? (0 or 1) */
 	char	   *ssl_min_protocol_version;	/* minimum TLS protocol version */
 	char	   *ssl_max_protocol_version;	/* maximum TLS protocol version */
 	char	   *target_session_attrs;	/* desired session properties */
diff --git a/src/test/kerberos/t/001_auth.pl b/src/test/kerberos/t/001_auth.pl
index bff26fda0c..0deb9bffc8 100644
--- a/src/test/kerberos/t/001_auth.pl
+++ b/src/test/kerberos/t/001_auth.pl
@@ -381,7 +381,7 @@ test_access(
 	'test1',
 	'SELECT gss_authenticated AND encrypted AND NOT credentials_delegated FROM pg_stat_gssapi WHERE pid = pg_backend_pid();',
 	0,
-	'gssencmode=prefer gssdelegation=enable',
+	'gssencmode=prefer gssdelegation=1',
 	'succeeds with GSS-encrypted access preferred with host hba and credentials not delegated even though asked for (ticket not forwardable)',
 	"connection authenticated: identity=\"test1\@$realm\" method=gss",
 	"connection authorized: user=$username database=$dbname application_name=$application GSS (authenticated=yes, encrypted=yes, delegated_credentials=no, principal=test1\@$realm)"
@@ -391,7 +391,7 @@ test_access(
 	

Make pgbench exit on SIGINT more reliably

[Tristan Partin]

Hello,

The way that pgbench handled SIGINT changed in
1d468b9ad81b9139b4a0b16b416c3597925af4b0. Unfortunately this had a
couple of unintended consequences, at least from what I can tell[1].

- CTRL-C no longer stops the program unless the right point in pgbench
  execution is hit
- pgbench no longer exits with a non-zero exit code

An easy reproduction of these problems is to run with a large scale
factor like: pgbench -i -s 50. Then try to CTRL-C the program.

The attached set of patches fixes this problem by allowing callers of
setup_cancel_handler() to attach a post-PQcancel callback. In this case,
we just call _exit(2). In addition, I noticed that psql had an EXIT_USER
constant, so I moved the common exit codes from src/bin/psql/settings.h
to src/include/fe_utils/exit_codes.h and made pgbench exit with
EXIT_USER.

[1]: 
https://www.postgresql.org/message-id/alpine.DEB.2.21.1910311939430.27369@lancre
-- 
Tristan Partin
Neon (https://neon.tech)
From 3cff6aec8f154eb8a47524efc15f16a6b7e95f37 Mon Sep 17 00:00:00 2001
From: Tristan Partin 
Date: Mon, 22 May 2023 08:07:37 -0500
Subject: [PATCH postgres v1 1/2] Move exit code definitions to fe_utils

This makes sharing exit code definitions much simpler.
---
 src/bin/psql/common.c |  3 ++-
 src/bin/psql/mainloop.c   |  1 +
 src/bin/psql/settings.h   | 13 -
 src/bin/psql/startup.c|  1 +
 src/include/fe_utils/exit_codes.h | 24 
 5 files changed, 28 insertions(+), 14 deletions(-)
 create mode 100644 src/include/fe_utils/exit_codes.h

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index c0e6e8e6ed..a36e188944 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -25,6 +25,7 @@
 #include "copy.h"
 #include "crosstabview.h"
 #include "fe_utils/cancel.h"
+#include "fe_utils/exit_codes.h"
 #include "fe_utils/mbprint.h"
 #include "fe_utils/string_utils.h"
 #include "portability/instr_time.h"
@@ -273,7 +274,7 @@ psql_cancel_callback(void)
 void
 psql_setup_cancel_handler(void)
 {
-	setup_cancel_handler(psql_cancel_callback);
+	setup_cancel_handler(psql_cancel_callback, NULL);
 }
 
 
diff --git a/src/bin/psql/mainloop.c b/src/bin/psql/mainloop.c
index 692c6db34c..98df0e0a97 100644
--- a/src/bin/psql/mainloop.c
+++ b/src/bin/psql/mainloop.c
@@ -10,6 +10,7 @@
 #include "command.h"
 #include "common.h"
 #include "common/logging.h"
+#include "fe_utils/exit_codes.h"
 #include "input.h"
 #include "mainloop.h"
 #include "mb/pg_wchar.h"
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 73d4b393bc..506d6db0a4 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -157,17 +157,4 @@ typedef struct _psqlSettings
 
 extern PsqlSettings pset;
 
-
-#ifndef EXIT_SUCCESS
-#define EXIT_SUCCESS 0
-#endif
-
-#ifndef EXIT_FAILURE
-#define EXIT_FAILURE 1
-#endif
-
-#define EXIT_BADCONN 2
-
-#define EXIT_USER 3
-
 #endif
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 5a28b6f713..877408f65b 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -19,6 +19,7 @@
 #include "common/logging.h"
 #include "common/string.h"
 #include "describe.h"
+#include "fe_utils/exit_codes.h"
 #include "fe_utils/print.h"
 #include "getopt_long.h"
 #include "help.h"
diff --git a/src/include/fe_utils/exit_codes.h b/src/include/fe_utils/exit_codes.h
new file mode 100644
index 00..d7136f8b50
--- /dev/null
+++ b/src/include/fe_utils/exit_codes.h
@@ -0,0 +1,24 @@
+/*
+ * psql - the PostgreSQL interactive terminal
+ *
+ * Copyright (c) 2000-2023, PostgreSQL Global Development Group
+ *
+ * src/include/fe_utils/exit_codes.h
+ */
+
+#ifndef EXIT_CODES_H
+#define EXIT_CODES_H
+
+#ifndef EXIT_SUCCESS
+#define EXIT_SUCCESS 0
+#endif
+
+#ifndef EXIT_FAILURE
+#define EXIT_FAILURE 1
+#endif
+
+#define EXIT_BADCONN 2
+
+#define EXIT_USER 3
+
+#endif
-- 
-- 
Tristan Partin
Neon (https://neon.tech)

From 4aa771e49095730cd1c5751c0517d147a2db6eeb Mon Sep 17 00:00:00 2001
From: Tristan Partin 
Date: Mon, 22 May 2023 08:19:27 -0500
Subject: [PATCH postgres v1 2/2] Add a post-PQcancel hook to
 setup_cancel_handler

This is a follow-up to 1d468b9ad81b9139b4a0b16b416c3597925af4b0. This
patch allowed pgbench to cancel server-side operations, but kept pgbench
from exiting until the only CancelRequested check was evaluated. In
addition, pgbench would not exit with a non-zero exit value given a
SIGINT.
---
 src/bin/pg_amcheck/pg_amcheck.c |  2 +-
 src/bin/pgbench/pgbench.c   | 17 +
 src/bin/scripts/clusterdb.c |  2 +-
 src/bin/scripts/reindexdb.c |  2 +-
 src/bin/scripts/vacuumdb.c  |  2 +-
 src/fe_utils/cancel.c   | 27 ++-
 src/include/fe_utils/cancel.h   |  5 -
 7 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c
index 68f8180c19..f3f31cde0f 100644
--- a/src/bin/pg_amcheck/pg_amcheck.c
+++ 

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

[Aleksander Alekseev]

Hi,

> Since no one seems to object so far I prepared the patch.

Turned out patch v1 fails on cfbot on Windows due to extra Assert I added [1]:

```
abort() has been calledTRAP: failed Assert("!IsUnderPostmaster"),
File: "../src/backend/storage/ipc/ipci.c", Line: 320, PID: 4040
abort() has been calledTRAP: failed Assert("!IsUnderPostmaster"),
File: "../src/backend/storage/ipc/ipci.c", Line: 320, PID: 3484
```

Which indicates that currently shmem_startup_hook **can** be called by
child processes on Windows. Not 100% sure if this is a desired
behavior considering the fact that it is inconsistent with the current
behavior on *nix systems.

Here is patch v2. Changes comparing to v1:

```
--- a/src/backend/storage/ipc/ipci.c
+++ b/src/backend/storage/ipc/ipci.c
@@ -311,15 +311,8 @@ CreateSharedMemoryAndSemaphores(void)
/*
 * Now give loadable modules a chance to set up their shmem allocations
 */
-   if (shmem_startup_hook)
-   {
-   /*
-* The following assert ensures that
shmem_startup_hook is going to be
-* called only by the postmaster, as promised in the
documentation.
-*/
-   Assert(!IsUnderPostmaster);
+   if (!IsUnderPostmaster && shmem_startup_hook)
shmem_startup_hook();
-   }
 }
```

Thoughts?

[1]: 
https://api.cirrus-ci.com/v1/artifact/task/4924036300406784/testrun/build/testrun/pg_stat_statements/regress/log/postmaster.log

-- 
Best regards,
Aleksander Alekseev


v2-0001-Clarify-the-usage-of-shmem_startup_hook.patch
Description: Binary data

Re: RFI: Extending the TOAST Pointer

[Aleksander Alekseev]

Hi,

> I see your point, but I do think we should also think about why we do
> the change.

Personally at the moment I care only about implementing compression
dictionaries on top of this, as is discussed in the corresponding
thread [1]. I'm going to need new fields in the TOAST pointer's
including (but not necessarily limited to) dictionary id.

As I understand, Nikita is interested in implementing 64-bit TOAST
pointers [2]. I must admit I didn't follow that thread too closely but
I can imagine the needs are similar.

Last but not least I remember somebody on the mailing list suggested
adding ZSTD compression support for TOAST, besides LZ4. Assuming I
didn't dream it, the proposal was rejected due to the limited amount
of free bits in ToastCompressionId. It was argued that two possible
combinations that are left should be treated with care and ZSTD will
not bring enough value to the users compared to LZ4.

These are 3 recent cases I could recall. This being said I think our
solution should be generic enough to cover possible future cases
and/or cases unknown to us yet.

[1]: 
https://postgr.es/m/CAJ7c6TM7%2BsTvwREeL74Y5U91%2B5ymNobRbOmnDRfdTonq9trZyQ%40mail.gmail.com
[2]: https://commitfest.postgresql.org/43/4296/

-- 
Best regards,
Aleksander Alekseev

Re: Naming of gss_accept_deleg

[Abhijit Menon-Sen]

At 2023-05-22 09:42:44 -0400, t...@sss.pgh.pa.us wrote:
>
> Alvaro Herrera  writes:
> > I noticed that the value that enables this feature at libpq client side
> > is 'enable'.  However, for other Boolean settings like sslsni,
> > keepalives, requiressl, sslcompression, the value that enables feature
> > is '1' -- we use strings only for "enum" type of settings.
> 
> > Also, it looks like connectOptions2() doesn't validate the string value.
> 
> Hmm, it certainly seems like this ought to accept exactly the
> same inputs as other libpq boolean settings.  I can take a look
> unless somebody else is already on it.

I'm working on it.

-- Abhijit

Re: Naming of gss_accept_deleg

[Tom Lane]

Alvaro Herrera  writes:
> I noticed that the value that enables this feature at libpq client side
> is 'enable'.  However, for other Boolean settings like sslsni,
> keepalives, requiressl, sslcompression, the value that enables feature
> is '1' -- we use strings only for "enum" type of settings.

> Also, it looks like connectOptions2() doesn't validate the string value.

Hmm, it certainly seems like this ought to accept exactly the
same inputs as other libpq boolean settings.  I can take a look
unless somebody else is already on it.

regards, tom lane

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

[Aleksander Alekseev]

Hi,

> Unless I missed something, I suggest updating the documentation and
> pg_stat_statements.c accordingly.

Since no one seems to object so far I prepared the patch.

-- 
Best regards,
Aleksander Alekseev


v1-0001-Clarify-the-usage-of-shmem_startup_hook.patch
Description: Binary data

Re: RFI: Extending the TOAST Pointer

[Matthias van de Meent]

On Sun, 21 May 2023, 15:39 Aleksander Alekseev,
 wrote:
>
> Hi,
>
> > We'd need to stop using the va_tag as length indicator, but I don't
> > think it's currently assumed to be a length indicator anyway (see
> > VARSIZE_EXTERNAL(ptr)). By not using the varatt_external struct
> > currently in use, we could be able to get down to <18B toast pointers
> > as well, though I'd consider that unlikely.
>
> Agree.
>
> Another thing we have to decide is what to do exactly in the scope of
> this thread.
>
> I imagine it as a refactoring that will find all the places that deal
> with current TOAST pointer and changes them to something like:
>
> ```
> switch(va_tag) {
>   case DEFAULT_VA_TAG( equals 18 ):
> default_toast_process_case_abc(...);
>   default:
> elog(ERROR, "Unknown TOAST tag")
> }
> ```

I'm not sure that we need all that.
Many places do some special handling for VARATT_IS_EXTERNAL because
decompressing or detoasting is expensive and doing that as late as
possible can be beneficial (e.g. EXPLAIN ANALYZE can run much faster
because we never detoast returned columns). But only very few of these
cases actually work on explicitly on-disk data: my IDE can't find any
uses of VARATT_IS_EXTERNAL_ONDISK (i.e. the actual TOASTed value)
outside the expected locations of the toast subsystems, amcheck, and
logical decoding (incl. the pgoutput plugin). I'm fairly sure we only
need to update existing paths in those subsystems to support another
format of external (but not the current VARTAG_ONDISK) data.

> So that next time somebody is going to need another type of TOAST
> pointer this person will have only to add a corresponding tag and
> handlers. (Something like "virtual methods" will produce a cleaner
> code but will also break branch prediction, so I don't think we should
> use those.)

Yeah, I'm also not super stoked about using virtual methods for a new
external toast implementation.

> I don't think we need an example of adding a new TOAST tag in scope of
> this work since the default one is going to end up being such an
> example.
>
> Does it make sense?

I see your point, but I do think we should also think about why we do
the change.

E.g.: Our current toast infra is built around 4 uint32 fields in the
toast pointer; but with this change in place we can devise a new toast
pointer that uses varint encoding on the length-indicating fields to
reduce the footprint of 18B to an expected 14 bytes.

Kind regards,

Matthias van de Meent
Neon, Inc.

Re: Add the ability to limit the amount of memory that can be allocated to backends.

[reid . thompson]

On Wed, 2023-05-17 at 23:07 -0400, reid.thomp...@crunchydata.com wrote:
> Thanks for the feedback.
> 
> I'm plannning to look at this. 
> 
> Is your benchmark something that I could utilize? I.E. is it a set of
> scripts or a standard test from somewhere that I can duplicate?
> 
> Thanks,
> Reid
> 
Hi Arne,

Followup to the above.

I experimented on my system regarding
"The simple query select * from generate_series(0, 1000) shows roughly 18.9 
% degradation on my test server."

My laptop:
32GB ram
11th Gen Intel(R) Core(TM) i7-11850H 8 cores/16 threads @ 2.50GHz (Max Turbo 
Frequency. 4.80 GHz ; Cache. 24 MB)
SSD -> Model: KXG60ZNV1T02 NVMe KIOXIA 1024GB (nvme)

I updated to latest master and rebased my patch branches.

I wrote a script to check out, build, install, init, and startup
master, patch 1, patch 1+2, patch 1+2 as master, pg-stats-memory, 
dev-max-memory, and dev-max-memory-unset configured with

../../configure --silent 
--prefix=/home/rthompso/src/git/postgres/install/${dir} --with-openssl 
--with-tcl --with-tclconfig=/usr/lib/tcl8.6 --with-perl --with-libxml 
--with-libxslt --with-python --with-gssapi --with-systemd --with-ldap 
--enable-nls

where $dir in  master, pg-stats-memory, and dev-max-memory,
dev-max-memory-unset.

The only change made to the default postgresql.conf was to have the
script add to the dev-max-memory instance the line
"max_total_backend_memory = 2048" before startup.
I did find one change in patch 2 that I pushed back into patch 1, this
should only impact the pg-stats-memory instance.

my .psqlrc turns timing on

I created a script where I can pass two instances to be compared.
It invokes
   psql -At -d postgres $connstr -P pager=off -c 'select * from 
generate_series(0, 1000)'
100 times on each of the 2 instances and calculates the AVG time and SD
for the 100 runs.  It then uses the AVG from each instance to calculate
the percentage difference.

Depending on the instance, my results differ from master from
negligible to ~5.5%.  Comparing master to itself had up to a ~2%
variation. See below.


12 runs comparing dev-max-memory 2048 VS master
Shows ~3% to 5.5% variation

Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1307.14 -> VER dev-max-memory 2048
1240.74 -> VER master
5.21218% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1315.99 -> VER dev-max-memory 2048
1245.64 -> VER master
5.4926% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1317.39 -> VER dev-max-memory 2048
1265.33 -> VER master
4.03141% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1313.52 -> VER dev-max-memory 2048
1256.69 -> VER master
4.42221% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1329.98 -> VER dev-max-memory 2048
1253.75 -> VER master
5.90077% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1314.47 -> VER dev-max-memory 2048
1245.6 -> VER master
5.38032% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1309.7 -> VER dev-max-memory 2048
1258.55 -> VER master
3.98326% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1322.16 -> VER dev-max-memory 2048
1248.94 -> VER master
5.69562% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1320.15 -> VER dev-max-memory 2048
1261.41 -> VER master
4.55074% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1345.22 -> VER dev-max-memory 2048
1280.96 -> VER master
4.8938% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1296.03 -> VER dev-max-memory 2048
1257.06 -> VER master
3.05277% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 2048 
and VER master
1319.5 -> VER dev-max-memory 2048
1252.34 -> VER master
5.22272% difference


12 showing dev-max-memory-unset VS master
Shows ~2.5% to 5% variation

Calculate average runtime percentage difference between VER dev-max-memory 
unset and VER master
1300.93 -> VER dev-max-memory unset
1235.12 -> VER master
5.18996% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 
unset and VER master
1293.57 -> VER dev-max-memory unset
1263.93 -> VER master
2.31789% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 
unset and VER master
1303.05 -> VER dev-max-memory unset
1258.11 -> VER master
3.50935% difference
--
Calculate average runtime percentage difference between VER dev-max-memory 
unset 

Re: running logical replication as the subscription owner

[Masahiko Sawada]

On Wed, May 17, 2023 at 10:10 AM Ajin Cherian  wrote:
>
> On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada  
> wrote:
> >
> > On Mon, May 15, 2023 at 5:44 PM Ajin Cherian  wrote:
> > >
> > > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian  wrote:
> > > >
> > > > If nobody else is working on this, I can come up with a patch to fix 
> > > > this
> > > >
> > >
> > > Attaching a patch which attempts to fix this.
> > >
> >
> > Thank you for the patch! I think we might want to have tests for it.
> >
> I have updated the patch with a test case as well.

Thank you for updating the patch! Here are review comments:

+   /*
+* Make sure that the copy command runs as the table owner, unless
+* the user has opted out of that behaviour.
+*/
+   run_as_owner = MySubscription->runasowner;
+   if (!run_as_owner)
+   SwitchToUntrustedUser(rel->rd_rel->relowner, );
+
/* Now do the initial data copy */
PushActiveSnapshot(GetTransactionSnapshot());

I think we should switch users before the acl check in
LogicalRepSyncTableStart().

---
+# Create a trigger on table alice.unpartitioned that writes
+# to a table that regress_alice does not have permission.
+$node_subscriber->safe_psql(
+   'postgres', qq(
+SET SESSION AUTHORIZATION regress_alice;
+CREATE OR REPLACE FUNCTION alice.alice_audit()
+RETURNS trigger AS
+\$\$
+   BEGIN
+   insert into public.admin_audit values(2);
+   RETURN NEW;
+   END;
+\$\$
+LANGUAGE 'plpgsql';
+CREATE TRIGGER ALICE_TRIGGER AFTER INSERT ON alice.unpartitioned FOR EACH ROW
+EXECUTE PROCEDURE alice.alice_audit();
+ALTER TABLE alice.unpartitioned ENABLE ALWAYS TRIGGER ALICE_TRIGGER;
+));

While this approach works, I'm not sure we really need a trigger for
this test. I've attached a patch for discussion that doesn't use
triggers for the regression tests. We create a new subscription owned
by a user who doesn't have the permission to the target table. The
test passes only if run_as_owner = true works.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com


fix_run_as_owner.patch
Description: Binary data

Re: Order changes in PG16 since ICU introduction

[Peter Eisentraut]

On 18.05.23 00:59, Jeff Davis wrote:

On Tue, 2023-05-16 at 20:23 -0700, Jeff Davis wrote:

Other than that, and I took your suggestions almost verbatim. Patch
attached. Thank you!


Attached new patch with a typo fix and a few other edits. I plan to
commit soon.


Some small follow-up on this patch:

Please put blank lines between




etc., matching existing style.

We usually don't capitalize the collation parameters like

CREATE COLLATION mycollation1 (PROVIDER = icu, LOCALE = 'ja-JP);

elsewhere in the documentation.

Table 24.2. ICU Collation Settings should probably be sorted by key, or 
at least by something.


All tables should referenced in the text, like "Table x.y shows this and 
that."  (Note that a table could float to a different page in some 
output formats, so just putting it into a section without some 
introductory text isn't sound.)


Table 24.1. ICU Collation Levels shows punctuation as level 4, which is 
only true in shifted mode, which isn't the default.  The whole business 
of treating variable collation elements is getting a bit lost in this 
description.  The kv option is described as "Classes of characters 
ignored during comparison at level 3.", which is effectively true but 
not the whole picture.

Re: Order changes in PG16 since ICU introduction

[Peter Eisentraut]

On 18.05.23 19:55, Jeff Davis wrote:

On Wed, 2023-05-17 at 19:59 -0400, Jonathan S. Katz wrote:

I did a quicker read through this time. LGTM overall. I like what you
did with the explanations around sensitivity (now it makes sense).


Committed, thank you.

There are a few things I don't understand that would be good to
document better:

* Rules. I still don't quite understand the use case: are these for
people inventing new languages? What is a plausible use case that isn't
covered by the existing locales and collation settings? Do rules make
sense for a database default collation? Are they for language experts
only or might an ordinary developer benefit from using them?


The rules are for setting whatever sort order you like.  Maybe you want 
to sort + before - or whatever.  It's like, if you don't like it, build 
your own.



* The collation types "phonebk", "emoji", etc.: are these variants of
particular locales, or do they make sense in multiple locales? I don't
know where they fit in or how to document them.


The k* settings are parametric settings, in that they transform the sort 
key in some algorithmic way.  The co settings are just everything else. 
They are not parametric, they are just some other sort order that 
someone spelled out explicitly.



* I don't understand what "kc" means if "ks" is not set to "level1".


There is an example here: 
https://peter.eisentraut.org/blog/2023/05/16/overview-of-icu-collation-settings#colcaselevel

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

[Zhijie Hou (Fujitsu)]

Hi,

On Monday, May 22, 2023 12:11 AM Andres Freund  wrote:
> On 2023-05-19 12:07:56 +0900, Kyotaro Horiguchi wrote:
> > At Thu, 18 May 2023 20:11:11 +0530, Bharath Rupireddy
>  wrote in
> > > > > +
> ConditionVariableInit(>physicalWALSndCV);
> > > > > + ConditionVariableInit(>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.
> 
> It's possible we could try to reduce the frequency by issuing wakeups only at
> specific points. The most obvious thing to do would be to wake only when
> waiting for more WAL or when crossing a page boundary, or such.
> Unfortunately
> that could easily lead to deadlocks, because the startup process might be
> blocked waiting for a lock, held by a backend doing logical decoding - which
> can't progress until the startup process wakes the backend up.

Just out of curiosity about the mentioned deadlock scenario, no comment on the
committed patch.

About "a backend doing logical decoding", do you mean the case when a user
start a backend and invoke pg_logical_slot_get_changes() to do the logical
decoding ? If so, it seems the logical decoding in a backend won't be waked up
by startup process because the backend won't be registered as a walsender so
the backend won't be found in WalSndWakeup().

Or do you mean the deadlock between the real logical walsender and startup
process ? (I might miss something) I think the logical decoding doesn't lock
the target user relation when decoding because it normally can get the needed
information from WAL. Besides, the walsender sometimes will lock the system
table(e.g. use RelidByRelfilenumber() to get the relid) but it will unlock it
after finishing systable scan.

So, if possible, would you be able to share some details about the deadlock
case you mentioned earlier? it's helpful as it can prevent similar problems in
the future development.

Best Regards,
Hou zj

Re: createuser --memeber and PG 16

[Nathan Bossart]

On Mon, May 22, 2023 at 08:42:28AM +0200, Peter Eisentraut wrote:
> On 21.05.23 19:07, Nathan Bossart wrote:
>> How do folks feel about keeping --role undocumented?  Should we give it a
>> mention in the docs for --member-of?
> 
> We made a point in this release to document deprecated options consistently.
> See commit 2f80c95740.

Alright.  Does the attached patch suffice?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/doc/src/sgml/ref/createuser.sgml b/doc/src/sgml/ref/createuser.sgml
index ba7ed1f853..5c34c62342 100644
--- a/doc/src/sgml/ref/createuser.sgml
+++ b/doc/src/sgml/ref/createuser.sgml
@@ -150,6 +150,7 @@ PostgreSQL documentation
  
   -g role
   --member-of=role
+  --role=role (deprecated)
   

 Specifies the new role should be automatically added as a member
diff --git a/src/bin/scripts/createuser.c b/src/bin/scripts/createuser.c
index 2d5e2452f7..0709491185 100644
--- a/src/bin/scripts/createuser.c
+++ b/src/bin/scripts/createuser.c
@@ -34,8 +34,7 @@ main(int argc, char *argv[])
 		{"no-createdb", no_argument, NULL, 'D'},
 		{"echo", no_argument, NULL, 'e'},
 		{"encrypted", no_argument, NULL, 'E'},
-		{"role", required_argument, NULL, 'g'}, /* kept for backward
- * compatibility */
+		{"role", required_argument, NULL, 'g'},
 		{"member-of", required_argument, NULL, 'g'},
 		{"host", required_argument, NULL, 'h'},
 		{"inherit", no_argument, NULL, 'i'},
@@ -423,6 +422,7 @@ help(const char *progname)
 	printf(_("  -D, --no-createdb role cannot create databases (default)\n"));
 	printf(_("  -e, --echoshow the commands being sent to the server\n"));
 	printf(_("  -g, --member-of=ROLE  new role will be a member of ROLE\n"));
+	printf(_("  --role=ROLE   (same as --member-of, deprecated)\n"));
 	printf(_("  -i, --inherit role inherits privileges of roles it is a\n"
 			 "member of (default)\n"));
 	printf(_("  -I, --no-inherit  role does not inherit privileges\n"));
diff --git a/src/bin/scripts/t/040_createuser.pl b/src/bin/scripts/t/040_createuser.pl
index 40452fcae3..9ca282181d 100644
--- a/src/bin/scripts/t/040_createuser.pl
+++ b/src/bin/scripts/t/040_createuser.pl
@@ -71,7 +71,7 @@ $node->issues_sql_like(
 $node->issues_sql_like(
 	[ 'createuser', '--role', 'regress_user1', 'regress_user11' ],
 	qr/statement: CREATE ROLE regress_user11 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,
-	'--role (for backward compatibility)');
+	'--role');
 $node->issues_sql_like(
 	[ 'createuser', '--member-of', 'regress_user1', 'regress_user12' ],
 	qr/statement: CREATE ROLE regress_user12 NOSUPERUSER NOCREATEDB NOCREATEROLE INHERIT LOGIN NOREPLICATION NOBYPASSRLS IN ROLE regress_user1;/,

Re: Adding SHOW CREATE TABLE

[Andrew Dunstan]

On 2023-05-22 Mo 05:24, Pavel Stehule wrote:



po 22. 5. 2023 v 7:19 odesílatel Kirk Wolak  napsal:

On Fri, May 19, 2023 at 1:08 PM 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



Andrew,
  Thanks for sharing that.  I reviewed your code. 10yrs, clearly
it's not working (as-is, but close), something interesting about the
structure you ended up in.  You check the type of the object and
redirect accordingly at the top level. Hmmm...
What I liked was that each type gets handled (I was focused on
"table"), but I realized similarities.

  I don't know what the group would think, but I like the thought
of calling this, and having it "Correct" to call the appropriate
function.
But not sure it will stand.  It does make obvious that some of
these should be spun out as "pg_get_typedef"..
pg_get_typedef
pg_get_domaindef
pg_get_sequencedef

  Finally, since you started this a while back, part of me is
"leaning" towards a function:
pg_get_columndef

  Which returns a properly formatted column for a table, type, or
domain? (one of the reasons for this, is that this is
the function with the highest probability to change, and
potentially the easiest to share reusability).

  Finally, I am curious about your opinion.  I noticed you used
the internal pg_ tables, versus the information_schema...
I am *thinking* that the information_schema will be more stable
over time... Thoughts?


I think inside the core, the information schema is never used.  And 
there was a performance issue (fixed in PostgreSQL 12), that blocked 
index usage.





A performant server side set of functions would be written in C and 
follow the patterns in ruleutils.c.



cheers


andrew

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

Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error

[Alvaro Herrera]

On 2023-May-20, Alexander Lakhin wrote:

> Starting from 038f586d5, the following script:
> echo "
> \startpipeline
> \endpipeline
> " >test.sql
> pgbench -n -M prepared -f test.sql
> 
> leads to the pgbench's segfault:

Hah, yeah, that's because an empty pipeline never calls the code to
allocate the flag array.  Here's the trivial fix.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)
>From 50685847e057eb8e7509293fdd81247eb49854ef Mon Sep 17 00:00:00 2001
From: Alvaro Herrera 
Date: Mon, 22 May 2023 13:45:47 +0200
Subject: [PATCH] Fix pgbench in prepared mode with empty pipeline

---
 src/bin/pgbench/pgbench.c| 44 
 src/bin/pgbench/t/001_pgbench_with_server.pl |  2 +
 2 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 7dbb2ed6a77..6caa84282c7 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3049,6 +3049,27 @@ chooseScript(TState *thread)
 	return i - 1;
 }
 
+/*
+ * Allocate space for 'prepared' flags of a script: we need one boolean
+ * for each command of each script.
+ */
+static void
+allocatePreparedFlags(CState *st)
+{
+	Assert(st->prepared == NULL);
+
+	st->prepared = pg_malloc(sizeof(bool *) * num_scripts);
+	for (int i = 0; i < num_scripts; i++)
+	{
+		ParsedScript *script = _script[i];
+		int			numcmds;
+
+		for (numcmds = 0; script->commands[numcmds] != NULL; numcmds++)
+			;
+		st->prepared[i] = pg_malloc0(sizeof(bool) * numcmds);
+	}
+}
+
 /*
  * Prepare the SQL command from st->use_file at command_num.
  */
@@ -3061,23 +3082,8 @@ prepareCommand(CState *st, int command_num)
 	if (command->type != SQL_COMMAND)
 		return;
 
-	/*
-	 * If not already done, allocate space for 'prepared' flags: one boolean
-	 * for each command of each script.
-	 */
 	if (!st->prepared)
-	{
-		st->prepared = pg_malloc(sizeof(bool *) * num_scripts);
-		for (int i = 0; i < num_scripts; i++)
-		{
-			ParsedScript *script = _script[i];
-			int			numcmds;
-
-			for (numcmds = 0; script->commands[numcmds] != NULL; numcmds++)
-;
-			st->prepared[i] = pg_malloc0(sizeof(bool) * numcmds);
-		}
-	}
+		allocatePreparedFlags(st);
 
 	if (!st->prepared[st->use_file][command_num])
 	{
@@ -3109,13 +3115,15 @@ prepareCommandsInPipeline(CState *st)
 	Assert(commands[st->command]->type == META_COMMAND &&
 		   commands[st->command]->meta == META_STARTPIPELINE);
 
+	if (!st->prepared)
+		allocatePreparedFlags(st);
+
 	/*
 	 * We set the 'prepared' flag on the \startpipeline itself to flag that we
 	 * don't need to do this next time without calling prepareCommand(), even
 	 * though we don't actually prepare this command.
 	 */
-	if (st->prepared &&
-		st->prepared[st->use_file][st->command])
+	if (st->prepared[st->use_file][st->command])
 		return;
 
 	for (j = st->command + 1; commands[j] != NULL; j++)
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 363a1ffabd5..f8ca8a922d1 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -790,6 +790,8 @@ $node->pgbench(
 		'001_pgbench_pipeline_prep' => q{
 -- test startpipeline
 \startpipeline
+\endpipeline
+\startpipeline
 } . "select 1;\n" x 10 . q{
 \endpipeline
 }
-- 
2.39.2

RE: Time delayed LR (WAS Re: logical replication restrictions)

[Hayato Kuroda (Fujitsu)]

Dear Amit,

Thank you for giving suggestions.

> > Dear hackers,
> >
> > I rebased and refined my PoC. Followings are the changes:
> >
> 
> 1. Is my understanding correct that this patch creates the delay files
> for each transaction? If so, did you consider other approaches such as
> using one file to avoid creating many files?

I have been analyzing the approach which uses only one file per subscription, 
per
your suggestion. Currently I'm not sure whether it is good approach or not, so 
could
you please give me any feedbacks?

TL;DR: rotating segment files like WALs may be used, but there are several 
issues.

# Assumption 

* Streamed txns are also serialized to the same permanent file, in the received 
order.
* No additional sorting is considered.

# Considerations

As a premise, applied txns must be removed from files, otherwise the disk 
becomes
full in some day and it leads PANIC.

## Naive approach - serialize all the changes to one large file

If workers continue to write received changes from the head naively, it may be
difficult to purge applied txns because there seems not to have a good way to
truncate the first part of the file. I could not find related functions in fd.h.

## Alternative approach - separate the file into segments

Alternative approach I came up with is that the file is divided into some 
segments
- like WAL - and remove it if all written txns are applied. It may work well in
non-streaming, 1pc case, but may not in other cases.

### Regarding the PREPARE transactions

At that time it is more likely to occur that the segment which contains the
actual txn is differ from the segment where COMMIT PREPARED. Hence the worker
must check all the remained segments to find the actual messages from them. 
Isn't
it inefficient? There is another approach that workers apply the PREPARE
immediately and spill to file only COMMIT PREPARED, but in this case the worker
have been acquiring the lock and never released it till delay is done.

### Regarding the streamed transactions

As for streaming case, chunks of txns are separated into several segments.
Hence the worker must check all the remained segments to find chunks messages
from them, same as above. Isn't it inefficient too?

Additionally, segments which have prepared or streamed transactions cannot be
removed, so even if the case many files may be generated and remained.

Anyway, it may be difficult to accept to stream in-progress transactions while
delaying the application. IIUC the motivation of steaming is to reduce the lag
between nodes, and it is opposite of this feature. So it might be okay, not 
sure.

### Regarding the publisher - timing to send schema may be fuzzy

Another issue is that the timing when publisher sends the schema information
cannot be determined on publisher itself. As discussed on hackers, publisher
must send schema information once per segment file, but it is controlled on
subscriber side.
I'm thinking that the walsender cannot recognize the changing of segments and
understand the timing to send them.

That's it. I'm very happy to get idea. 

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

RE: [PoC] pg_upgrade: allow to upgrade publisher node

[Hayato Kuroda (Fujitsu)]

Dear Wang,

Thank you for reviewing! PSA new version.

> For patches 0001
> 
> 1. The latest patch set fails to apply because the new commit (0245f8d) in 
> HEAD.

I didn't notice that. Thanks, fixed.

> 2. In file pg_dump.h.
> ```
> +/*
> + * The LogicalReplicationSlotInfo struct is used to represent replication
> + * slots.
> + *
> + * XXX: add more attributes if needed
> + */
> +typedef struct _LogicalReplicationSlotInfo
> +{
> + DumpableObject dobj;
> + char   *plugin;
> + char   *slottype;
> + booltwophase;
> +} LogicalReplicationSlotInfo;
> ```
> 
> Do we need the structure member "slottype"? It seems we do not use "slottype"
> because we only dump logical replication slot.

As you said, this attribute is not needed. This is a garbage of previous 
efforts.
Removed.

> For patch 0002
> 
> 3. In the function SaveSlotToPath
> ```
> - /* and don't do anything if there's nothing to write */
> - if (!was_dirty)
> + /*
> +  * and don't do anything if there's nothing to write, unless it's this 
> is
> +  * called for a logical slot during a shutdown checkpoint, as we want to
> +  * persist the confirmed_flush_lsn in that case, even if that's the only
> +  * modification.
> +  */
> + if (!was_dirty && !is_shutdown && !SlotIsLogical(slot))
> ```
> It seems that the code isn't consistent with our expectation.
> If this is called for a physical slot during a shutdown checkpoint and there's
> nothing to write, I think it will also persist physical slots to disk.

You meant to say that we should not change handlings for physical case, right?

> For patch 0003
> 
> 4. In the function check_for_parameter_settings
> ```
> + /* --include-logical-replication-slots can be used since PG 16. */
> + if (GET_MAJOR_VERSION(new_cluster->major_version < 1600))
> + return;
> ```
> It seems that there is a slight mistake (the input of GET_MAJOR_VERSION) in 
> the
> if-condition:
> GET_MAJOR_VERSION(new_cluster->major_version < 1600)
> ->
> GET_MAJOR_VERSION(new_cluster->major_version) <= 1500
> 
> Please also check the similar if-conditions in the below two functions
> check_for_confirmed_flush_lsn (in 0003 patch)
> check_are_logical_slots_active (in 0004 patch)

Done. I grepped with GET_MAJOR_VERSION, and confirmed they were fixed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED



v15-0001-pg_upgrade-Add-include-logical-replication-slots.patch
Description:  v15-0001-pg_upgrade-Add-include-logical-replication-slots.patch


v15-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch
Description:  v15-0002-Always-persist-to-disk-logical-slots-during-a-sh.patch


v15-0003-pg_upgrade-Add-check-function-for-include-logica.patch
Description:  v15-0003-pg_upgrade-Add-check-function-for-include-logica.patch


v15-0004-Change-the-method-used-to-check-logical-replicat.patch
Description:  v15-0004-Change-the-method-used-to-check-logical-replicat.patch

unnecessary #include "pg_getopt.h"?

[torikoshia]

Hi,

While working on [1], I thought there seems to be unnecessary #include 
"pg_getopt.h".
getopt_long.h has already included pg_getopt.h, but some files include 
both getopt.h and getopt_long.h.



[1] 
https://www.postgresql.org/message-id/flat/d660ef741ce3d82f3b4283f1cafd5...@oss.nttdata.com

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATIONFrom 0c11610505a3f25d8d65e9f8969328bba89f5c44 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi 
Date: Mon, 22 May 2023 17:37:25 +0900
Subject: [PATCH v1] Remove unnecessary include pg_getopt.h

---
 contrib/oid2name/oid2name.c | 1 -
 contrib/vacuumlo/vacuumlo.c | 1 -
 src/bin/pg_checksums/pg_checksums.c | 1 -
 src/bin/pg_controldata/pg_controldata.c | 1 -
 src/bin/pg_resetwal/pg_resetwal.c   | 1 -
 5 files changed, 5 deletions(-)

diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index e8c1e2c97b..32b2b43608 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -15,7 +15,6 @@
 #include "common/string.h"
 #include "getopt_long.h"
 #include "libpq-fe.h"
-#include "pg_getopt.h"
 
 /* an extensible array to keep track of elements to show */
 typedef struct
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index 8941262731..852486dc11 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -27,7 +27,6 @@
 #include "common/string.h"
 #include "getopt_long.h"
 #include "libpq-fe.h"
-#include "pg_getopt.h"
 
 #define BUFSIZE			1024
 
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 19eb67e485..03678851ef 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -27,7 +27,6 @@
 #include "common/logging.h"
 #include "fe_utils/option_utils.h"
 #include "getopt_long.h"
-#include "pg_getopt.h"
 #include "storage/bufpage.h"
 #include "storage/checksum.h"
 #include "storage/checksum_impl.h"
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index c390ec51ce..52d7c170e1 100644
--- a/src/bin/pg_controldata/pg_controldata.c
+++ b/src/bin/pg_controldata/pg_controldata.c
@@ -27,7 +27,6 @@
 #include "common/controldata_utils.h"
 #include "common/logging.h"
 #include "getopt_long.h"
-#include "pg_getopt.h"
 
 static void
 usage(const char *progname)
diff --git a/src/bin/pg_resetwal/pg_resetwal.c b/src/bin/pg_resetwal/pg_resetwal.c
index e7ef2b8bd0..58ad79d38f 100644
--- a/src/bin/pg_resetwal/pg_resetwal.c
+++ b/src/bin/pg_resetwal/pg_resetwal.c
@@ -56,7 +56,6 @@
 #include "common/restricted_token.h"
 #include "common/string.h"
 #include "getopt_long.h"
-#include "pg_getopt.h"
 #include "storage/large_object.h"
 
 static ControlFileData ControlFile; /* pg_control values */
-- 
2.39.2

Re: Adding SHOW CREATE TABLE

[Pavel Stehule]

po 22. 5. 2023 v 7:19 odesílatel Kirk Wolak  napsal:

> On Fri, May 19, 2023 at 1:08 PM 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
>> 
>> 
>>
> Andrew,
>   Thanks for sharing that.  I reviewed your code.  10yrs, clearly it's not
> working (as-is, but close), something interesting about the
> structure you ended up in.  You check the type of the object and redirect
> accordingly at the top level.  Hmmm...
> What I liked was that each type gets handled (I was focused on "table"),
> but I realized similarities.
>
>   I don't know what the group would think, but I like the thought of
> calling this, and having it "Correct" to call the appropriate function.
> But not sure it will stand.  It does make obvious that some of these
> should be spun out as "pg_get_typedef"..
> pg_get_typedef
> pg_get_domaindef
> pg_get_sequencedef
>
>   Finally, since you started this a while back, part of me is "leaning"
> towards a function:
> pg_get_columndef
>
>   Which returns a properly formatted column for a table, type, or domain?
> (one of the reasons for this, is that this is
> the function with the highest probability to change, and potentially the
> easiest to share reusability).
>
>   Finally, I am curious about your opinion.  I noticed you used the
> internal pg_ tables, versus the information_schema...
> I am *thinking* that the information_schema will be more stable over
> time... Thoughts?
>

I think inside the core, the information schema is never used.  And there
was a performance issue (fixed in PostgreSQL 12), that blocked index usage.

Regards

Pavel



> Thank you for sharing your thoughts...
> Kirk...
>
>

Re: Allow pg_archivecleanup to remove backup history files

[torikoshia]

On 2023-05-15 15:22, Michael Paquier wrote:

On Mon, May 15, 2023 at 03:16:46PM +0900, torikoshia wrote:

On 2023-05-15 09:18, Michael Paquier wrote:

How about plugging in some long options, and use something more
explicit like --clean-backup-history?


Agreed.


If you begin to implement that, it seems to me that this should be
shaped with a first separate patch that refactors the code to use
getopt_long(), and a second patch for the proposed feature that builds
on top of it.

Thanks for your advice, attached patches.

--
Regards,

--
Atsushi Torikoshi
NTT DATA CORPORATIONFrom 814b7351f14626f02c13b21d1a6737461117e5d0 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi 
Date: Mon, 22 May 2023 17:37:25 +0900
Subject: [PATCH v3 1/2] Introduce pg_archivecleanup into getopt_long

This patch is a preliminary step to add an easy-to-understand option
to delete backup history files, but it also adds long options to
the existing options.
---
 doc/src/sgml/ref/pgarchivecleanup.sgml|  5 -
 src/bin/pg_archivecleanup/pg_archivecleanup.c | 20 ---
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/doc/src/sgml/ref/pgarchivecleanup.sgml b/doc/src/sgml/ref/pgarchivecleanup.sgml
index 635e7c7685..09991c2fcd 100644
--- a/doc/src/sgml/ref/pgarchivecleanup.sgml
+++ b/doc/src/sgml/ref/pgarchivecleanup.sgml
@@ -95,6 +95,7 @@ pg_archivecleanup:  removing file "archive/00010037000E"
 
  
   -d
+  --debug
   

 Print lots of debug logging output on stderr.
@@ -104,6 +105,7 @@ pg_archivecleanup:  removing file "archive/00010037000E"
 
  
   -n
+  --dry-run
   

 Print the names of the files that would have been removed on stdout (performs a dry run).
@@ -122,7 +124,8 @@ pg_archivecleanup:  removing file "archive/00010037000E"
  
 
  
-  -x extension
+  -x extension
+  --strip-extension=extension
   

 Provide an extension
diff --git a/src/bin/pg_archivecleanup/pg_archivecleanup.c b/src/bin/pg_archivecleanup/pg_archivecleanup.c
index 7726d05149..62914bdfa7 100644
--- a/src/bin/pg_archivecleanup/pg_archivecleanup.c
+++ b/src/bin/pg_archivecleanup/pg_archivecleanup.c
@@ -17,7 +17,7 @@
 
 #include "access/xlog_internal.h"
 #include "common/logging.h"
-#include "pg_getopt.h"
+#include "getopt_long.h"
 
 const char *progname;
 
@@ -252,11 +252,11 @@ usage(void)
 	printf(_("Usage:\n"));
 	printf(_("  %s [OPTION]... ARCHIVELOCATION OLDESTKEPTWALFILE\n"), progname);
 	printf(_("\nOptions:\n"));
-	printf(_("  -d generate debug output (verbose mode)\n"));
-	printf(_("  -n dry run, show the names of the files that would be removed\n"));
-	printf(_("  -V, --version  output version information, then exit\n"));
-	printf(_("  -x EXT clean up files if they have this extension\n"));
-	printf(_("  -?, --help show this help, then exit\n"));
+	printf(_("  -d, --debug   generate debug output (verbose mode)\n"));
+	printf(_("  -n, --dry-run dry run, show the names of the files that would be removed\n"));
+	printf(_("  -V, --version output version information, then exit\n"));
+	printf(_("  -x --strip-extension=EXT  clean up files if they have this extension\n"));
+	printf(_("  -?, --helpshow this help, then exit\n"));
 	printf(_("\n"
 			 "For use as archive_cleanup_command in postgresql.conf:\n"
 			 "  archive_cleanup_command = 'pg_archivecleanup [OPTION]... ARCHIVELOCATION %%r'\n"
@@ -274,6 +274,12 @@ usage(void)
 int
 main(int argc, char **argv)
 {
+	static struct option long_options[] = {
+		{"debug", no_argument, NULL, 'd'},
+		{"dry-run", no_argument, NULL, 'n'},
+		{"strip-extension", required_argument, NULL, 'x'},
+		{NULL, 0, NULL, 0}
+	};
 	int			c;
 
 	pg_logging_init(argv[0]);
@@ -294,7 +300,7 @@ main(int argc, char **argv)
 		}
 	}
 
-	while ((c = getopt(argc, argv, "dnx:")) != -1)
+	while ((c = getopt_long(argc, argv, "dnx:", long_options, NULL)) != -1)
 	{
 		switch (c)
 		{

base-commit: ac298d3cb56b015acd40d2e015e07a87d8aff124
-- 
2.39.2

From 6017a774691cf52f7f51b817dee26db3bc7879c0 Mon Sep 17 00:00:00 2001
From: Atsushi Torikoshi 
Date: Mon, 22 May 2023 17:41:57 +0900
Subject: [PATCH v3 2/2] Allow pg_archivecleanup to remove backup history files

Backup history files are just few bytes, but it can be noisy for the
eye to see a large accumulation of history files mixed with the WAL
segments.
This patch adds a new option to remove files including backup
history files older oldestkeptwalfile.
---
 doc/src/sgml/ref/pgarchivecleanup.sgml| 13 +++
 src/bin/pg_archivecleanup/pg_archivecleanup.c | 88 +++
 .../t/010_pg_archivecleanup.pl| 36 +---
 3 files changed, 91 insertions(+), 46 deletions(-)

diff --git a/doc/src/sgml/ref/pgarchivecleanup.sgml b/doc/src/sgml/ref/pgarchivecleanup.sgml
index 09991c2fcd..e0efabd989 100644
--- 

Re: Naming of gss_accept_deleg

[Alvaro Herrera]

I noticed that the value that enables this feature at libpq client side
is 'enable'.  However, for other Boolean settings like sslsni,
keepalives, requiressl, sslcompression, the value that enables feature
is '1' -- we use strings only for "enum" type of settings.

Also, it looks like connectOptions2() doesn't validate the string value.

-- 
Álvaro Herrera   48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
 really, I see PHP as like a strange amalgamation of C, Perl, Shell
 inflex: you know that "amalgam" means "mixture with mercury",
   more or less, right?
 i.e., "deadly poison"

Re: Memory leak from ExecutorState context?

[Jehan-Guillaume de Rorthais]

On Fri, 19 May 2023 17:23:56 +0200
Tomas Vondra  wrote:
> On 5/19/23 00:27, Melanie Plageman wrote:
> > v10 LGTM.  
> 
> Thanks!
> 
> I've pushed 0002 and 0003, after some general bikeshedding and minor
> rewording (a bit audacious, admittedly).

Thank you Melanie et Tomas for your help, review et commit!

Re: Avoiding another needless ERROR during nbtree page deletion

[Heikki Linnakangas]

On 20/05/2023 05:17, Peter Geoghegan wrote:

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.


Yeah, let's be consistent. Any idea what might cause this corruption?


Attached patch fixes it.
+   /*
+* Validate target's right sibling page's left link points back to 
target.
+*
+* This is known to fail in the field in the presence of index 
corruption,
+* so we go to the trouble of avoiding a hard ERROR.  This is fairly 
close
+* to what we did earlier on when we located the target's left sibling
+* (iff target has a left sibling).
+*/


This comment notes that this is similar to what we did with the left 
sibling, but there isn't really any mention at the left sibling code 
about avoiding hard ERRORs. Feels a bit backwards. Maybe move the 
comment about avoiding the hard ERROR to where the left sibling is 
handled. Or explain it in the function comment and just have short 
"shouldn't happen, but avoid hard ERROR if the index is corrupt" comment 
here.



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).


You could check that the left sibling is indeed a half-dead page.


ereport(DEBUG1,
(errcode(ERRCODE_NO_DATA),
 errmsg("block %u is not leftmost in index \"%s\"",
current, RelationGetRelationName(state->rel;


ERRCODE_NO_DATA doesn't look right. Let's just leave out the errcode.

--
Heikki Linnakangas
Neon (https://neon.tech)

Re: createuser --memeber and PG 16

[Peter Eisentraut]

On 21.05.23 19:07, Nathan Bossart wrote:

How do folks feel about keeping --role undocumented?  Should we give it a
mention in the docs for --member-of?


We made a point in this release to document deprecated options 
consistently.  See commit 2f80c95740.