Re: How to add a new operator for parser?

[jacktby jacktby]

> 2023年8月6日 13:18，Julien Rouhaud  写道：
> 
> On Sun, 6 Aug 2023, 12:34 jacktby jacktby,  > wrote:
>> I’m trying to add a new operator for my pg application like greater_equals 
>> called “<~>", how many files I need to 
>> modify and how to do it? Can you give me an example?
> 
> 
> you can look at some contrib for some examples of custom operator (and custom 
> datatype) implementation, like citext or btree_gin/gist
I need to build a new datatype. It can contains different datatypes, like 
‘(1,’a’,2.0)’,it’s a (interger,string,float) tuple type, and Then I need to add 
operator for it. How should I do?

logical decoding issue with concurrent ALTER TYPE

[Masahiko Sawada]

Hi all,

A colleague Drew Callahan (in CC) has discovered that the logical
decoding doesn't handle syscache invalidation messages properly that
are generated by other transactions. Here is example (I've attached a
patch for isolation test),

-- Setup
CREATE TYPE comp AS (f1 int, f2 text);
CREATE TABLE tbl3(val1 int, val2 comp);
SELECT pg_create_logical_replication_slot('s', 'test_decoding');

-- Session 1
BEGIN;
INSERT INTO tbl3 (val1, val2) VALUES (1, (1, '2'));

-- Session 2
ALTER TYPE comp ADD ATTRIBUTE f3 int;

INSERT INTO tbl3 (val1, val2) VALUES (1, (1, '2', 3));
COMMIT;

pg_logical_slot_get_changes() returns:

BEGIN
table public.tbl3: INSERT: val1[integer]:1 val2[comp]:'(1,2)'
table public.tbl3: INSERT: val1[integer]:1 val2[comp]:'(1,2)'
COMMIT

However, the logical decoding should reflect the result of ALTER TYPE
and the val2 of the second INSERT output should be '(1,2,3)'.

The root cause of this behavior is that while ALTER TYPE can be run
concurrently to INSERT, the logical decoding doesn't handle cache
invalidation properly, and it got a cache hit of stale data (of
typecache in this case). Unlike snapshots that are stored in the
transaction’s reorder buffer changes, the invalidation messages of
other transactions are not distributed. As a result, the snapshot
becomes moot when we get a cache hit of stale data due to not
processing the invalidation messages again. This is not an issue for
ALTER TABLE and the like due to 2 phase locking and taking an
AccessExclusive lock. The issue with DMLs and ALTER TYPE has been
discovered but there might be other similar cases.

As far as I tested, this issue has existed since v9.4, where the
logical decoding was introduced, so it seems to be a long-standing
issue.

The simplest fix would be to invalidate all caches when setting a new
historical snapshot (i.e. applying CHANGE_INTERNAL_SNAPSHOT) but we
end up invalidating other caches unnecessarily too.

A better fix would be that when decoding the commit of a catalog
changing transaction, we distribute invalidation messages to other
concurrent transactions, like we do for snapshots. But we might not
need to distribute all types of invalidation messages, though.

Regards,

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


logical-decoding-ALTER-TYPE.patch
Description: Binary data

Re: How to add a new operator for parser?

[Julien Rouhaud]

On Sun, 6 Aug 2023, 12:34 jacktby jacktby,  wrote:

> I’m trying to add a new operator for my pg application like greater_equals
> called “<~>", how many files I need to
> modify and how to do it? Can you give me an example?
>

you can look at some contrib for some examples of custom operator (and
custom datatype) implementation, like citext or btree_gin/gist

>

How to add a new operator for parser?

[jacktby jacktby]

I’m trying to add a new operator for my pg application like greater_equals 
called “<~>", how many files I need to 
modify and how to do it? Can you give me an example?

Re: POC, WIP: OR-clause support for indexes

[Peter Geoghegan]

On Thu, Aug 3, 2023 at 12:47 PM Alena Rybakina  wrote:
> It's all right. I understand your position)

Okay, good to know.  :-)

> I also agree to try to find other optimization cases and generalize them.

Good idea. Since the real goal is to "get a working flow of
information", the practical value of trying to get it working with
other clauses seems to be of secondary importance.

> To be honest, I tried to fix it many times by calling the function to
> calculate selectivity, and each time the result of the estimate did not
> change. I didn't have any problems in this part after moving the
> transformation to the parsing stage. I even tried to perform this
> transformation at the planning stage (to the preprocess_qual_conditions
> function), but I ran into the same problem there as well.
>
> To tell the truth, I think I'm ready to investigate this problem again
> (maybe I'll be able to see it differently or really find that I missed
> something in previous times).

The optimizer will itself do a limited form of "normalizing to CNF".
Are you familiar with extract_restriction_or_clauses(), from
orclauses.c? Comments above the function have an example of how this
can work:

 * Although a join clause must reference multiple relations overall,
 * an OR of ANDs clause might contain sub-clauses that reference just one
 * relation and can be used to build a restriction clause for that rel.
 * For example consider
 *  WHERE ((a.x = 42 AND b.y = 43) OR (a.x = 44 AND b.z = 45));
 * We can transform this into
 *  WHERE ((a.x = 42 AND b.y = 43) OR (a.x = 44 AND b.z = 45))
 *  AND (a.x = 42 OR a.x = 44)
 *  AND (b.y = 43 OR b.z = 45);
 * which allows the latter clauses to be applied during the scans of a and b,
 * perhaps as index qualifications, and in any case reducing the number of
 * rows arriving at the join.  In essence this is a partial transformation to
 * CNF (AND of ORs format).  It is not complete, however, because we do not
 * unravel the original OR --- doing so would usually bloat the qualification
 * expression to little gain.

Of course this immediately makes me wonder: shouldn't your patch be
able to perform an additional transformation here? You know, by
transforming "a.x = 42 OR a.x = 44" into "a IN (42, 44)"? Although I
haven't checked for myself, I assume that this doesn't happen right
now, since your patch currently performs all of its transformations
during parsing.

I also noticed that the same comment block goes on to say something
about "clauselist_selectivity's inability to recognize redundant
conditions". Perhaps that is relevant to the problems you were having
with selectivity estimation, back when the code was in
preprocess_qual_conditions() instead? I have no reason to believe that
there should be any redundancy left behind by your transformation, so
this is just one possibility to consider.

Separately, the commit message of commit 25a9e54d2d says something
about how the planner builds RestrictInfos, which seems
possibly-relevant. That commit enhanced extended statistics for OR
clauses, so the relevant paragraph describes a limitation of extended
statistics with OR clauses specifically. I'm just guessing, but it
still seems like it might be relevant to the problem you ran into with
selectivity estimation. Another possibility to consider.

BTW, I sometimes use RR to help improve my understanding of the planner:

https://wiki.postgresql.org/wiki/Getting_a_stack_trace_of_a_running_PostgreSQL_backend_on_Linux/BSD#Recording_Postgres_using_rr_Record_and_Replay_Framework

The planner has particularly complicated control flow, which has
unique challenges -- just knowing where to begin can be difficult
(unlike most other areas). I find that setting watchpoints to see when
and where the planner modifies state using RR is far more useful than
it would be with regular GDB. Once I record a query, I find that I can
"map out" what happens in the planner relatively easily.

-- 
Peter Geoghegan

Re: PG 16 draft release notes ready

[Noah Misch]

On Thu, May 18, 2023 at 04:49:47PM -0400, Bruce Momjian wrote:
>   https://momjian.us/pgsql_docs/release-16.html

> 
> 
> 
> 
> Restrict the privileges of CREATEROLE roles (Robert Haas)
> 
> 
> 
> Previously roles with CREATEROLE privileges could change many aspects of any 
> non-superuser role.  Such changes, including adding members, now require the 
> role requesting the change to have ADMIN OPTION
> permission.
> 
> 
> 
> 
> 
> 
> 
> Improve logic of CREATEROLE roles ability to control other roles (Robert Haas)
> 
> 
> 
> For example, they can change the CREATEDB, REPLICATION, and BYPASSRLS 
> properties only if they also have those permissions.
> 
> 

CREATEROLE is a radically different feature in v16.  In v15-, it was an
almost-superuser.  In v16, informally speaking, it can create and administer
its own collection of roles, but it can't administer roles outside its
collection or grant memberships or permissions not offered to itself.  Hence,
let's move these two into the incompatibilities section.  Let's also merge
them, since f1358ca52 is just doing to clauses like CREATEDB what cf5eb37c5
did to role memberships.

> 
> 
> 
> 
> Allow GRANT to control role inheritance behavior (Robert Haas)
> 
> 
> 
> By default, role inheritance is controlled by the inheritance status of the 
> member role.  The new GRANT clauses WITH INHERIT and WITH ADMIN can now 
> override this.
> 
> 
> 
> 
> 
> 
> 
> Allow roles that create other roles to automatically inherit the new role's 
> rights or SET ROLE to the new role (Robert Haas, Shi Yu)
> 
> 
> 
> This is controlled by server variable createrole_self_grant.
> 
> 

Similarly, v16 radically changes the CREATE ROLE ... WITH INHERIT clause.  The
clause used to "change the behavior of already-existing grants."  Let's merge
these two and move the combination to the incompatibilities section.

> Remove libpq support for SCM credential authentication (Michael Paquier)

Since the point of removing it is the deep unlikelihood of anyone using it, I
wouldn't list this in "incompatibilities".

> Deprecate createuser option --role (Nathan Bossart)

This is indeed a deprecation, not a removal.  By the definition of
"deprecate", it's not an incompatibility.

Re: initdb caching during tests

[Andres Freund]

Hi,

On 2023-08-05 16:58:38 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Times for running all tests under meson, on my workstation (20 cores / 40
> > threads):
> 
> > cassert build -O2:
> 
> > Before:
> > real0m44.638s
> > user7m58.780s
> > sys 2m48.773s
> 
> > After:
> > real0m38.938s
> > user2m37.615s
> > sys 2m0.570s
> 
> Impressive results.  Even though your bottom-line time doesn't change that
> much

Unfortunately we have a few tests that take quite a while - for those the
initdb removal doesn't make that much of a difference. Particularly because
this machine has enough CPUs to not be fully busy except for the first few
seconds...

E.g. for a run with the patch applied:

258/265 postgresql:pg_basebackup / pg_basebackup/010_pg_basebackup  
   OK   16.58s   187 subtests passed
259/265 postgresql:subscription / subscription/100_bugs 
   OK6.69s   12 subtests passed
260/265 postgresql:regress / regress/regress
   OK   24.95s   215 subtests passed
261/265 postgresql:ssl / ssl/001_ssltests   
   OK7.97s   205 subtests passed
262/265 postgresql:pg_dump / pg_dump/002_pg_dump
   OK   19.65s   11262 subtests passed
263/265 postgresql:recovery / recovery/027_stream_regress   
   OK   29.34s   6 subtests passed
264/265 postgresql:isolation / isolation/isolation  
   OK   33.94s   112 subtests passed
265/265 postgresql:pg_upgrade / pg_upgrade/002_pg_upgrade   
   OK   38.22s   18 subtests passed

The pg_upgrade test is faster in isolation (29s), but not that much. The
overall runtime is reduces due to the reduced "competing" cpu usage, but other
than that...


Looking at where the time is spent when running the pg_upgrade test on its own:

grep -E '^\[' testrun/pg_upgrade/002_pg_upgrade/log/regress_log_002_pg_upgrade 
|sed -E -e 's/.*\(([0-9.]+)s\)(.*)/\1 \2/g'|sort -n -r

cassert:
13.094  ok 5 - regression tests pass
6.147  ok 14 - run of pg_upgrade for new instance
2.340  ok 6 - dump before running pg_upgrade
1.638  ok 17 - dump after running pg_upgrade
1.375  ok 12 - run of pg_upgrade --check for new instance
0.798  ok 1 - check locales in original cluster
0.371  ok 9 - invalid database causes failure status (got 1 vs expected 1)
0.149  ok 7 - run of pg_upgrade --check for new instance with incorrect binary 
path
0.131  ok 16 - check that locales in new cluster match original cluster

optimized:
8.372  ok 5 - regression tests pass
3.641  ok 14 - run of pg_upgrade for new instance
1.371  ok 12 - run of pg_upgrade --check for new instance
1.104  ok 6 - dump before running pg_upgrade
0.636  ok 17 - dump after running pg_upgrade
0.594  ok 1 - check locales in original cluster
0.359  ok 9 - invalid database causes failure status (got 1 vs expected 1)
0.148  ok 7 - run of pg_upgrade --check for new instance with incorrect binary 
path
0.127  ok 16 - check that locales in new cluster match original cluster


The time for "dump before running pg_upgrade" is misleadingly high - there's
no output between starting initdb and the dump, so the timing includes initdb
and a bunch of other work.  But it's still not fast (1.637s) after.

A small factor is that the initdb times are not insignificant, because the
template initdb can't be used due to a bunch of parameters passed to initdb :)


> the big reduction in CPU time should translate to a nice speedup on slower
> buildfarm animals.

Yea. It's a particularly large win when using valgrind. Under valgrind, a very
large portion of the time for many tests is just spent doing initdb... So I am
hoping to see some nice gains for skink.

Greetings,

Andres Freund

Re: initdb caching during tests

[Tom Lane]

Andres Freund  writes:
> Times for running all tests under meson, on my workstation (20 cores / 40
> threads):

> cassert build -O2:

> Before:
> real  0m44.638s
> user  7m58.780s
> sys   2m48.773s

> After:
> real  0m38.938s
> user  2m37.615s
> sys   2m0.570s

Impressive results.  Even though your bottom-line time doesn't change that
much, the big reduction in CPU time should translate to a nice speedup
on slower buildfarm animals.

(Disclaimer: I've not read the patch.)

regards, tom lane

ci: Improve macos startup using a cached macports installation

[Andres Freund]

Hi,

We have some issues with CI on macos and windows being too expensive (more on
that soon in a separate email). For macos most of the obviously wasted time is
spent installing packages with homebrew. Even with the package downloads being
cached, it takes about 1m20s to install them.  We can't just cache the whole
homebrew installation, because it contains a lot of pre-installed packages.

After a bunch of experimenting, I found a way to do this a lot faster: The
attached patch installs macports and installs our dependencies from
that. Because there aren't pre-existing macports packages, we can just cache
the whole thing.  Doing so naively wouldn't yield that much of a speedup,
because it takes a while to unpack a tarball (or whatnot) with as many files
as our dependencies have - that's a lot of filesystem metadata
operations. Instead the script creates a HFS+ filesystem in a file and caches
that - that's mounted within a few seconds. To further keep the size in check,
that file is compressed with zstd in the cache.

As macports has a package for IPC::Run and IO::Pty, we can use those instead
of the separate cache we had for the perl installation.

After the patch, the cached case takes ~5s to "install" and the cache is half
the size than the one for homebrew.

The comparison isn't entirely fair, because I used the occasion to not install
'make' (since meson is used for building) and llvm (we didn't enable it for
the build anyway). That gain is a bit smaller without that, but still
significant.


An alternative implementation would be to create the "base" .dmg file outside
of CI and download it onto the CI instances. But I didn't want to figure out
the hosting situation for such files, so I thought this was the best
near-medium term path.

Greetings,

Andres Freund
>From e0f64949e7b2b2aca4398acb3bfa8e859857e910 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Thu, 3 Aug 2023 23:29:13 -0700
Subject: [PATCH v1] ci: macos: used cached macports install

This substantially speeds up the mac CI time.

Author:
Reviewed-by:
Discussion: https://postgr.es/m/
Backpatch:
---
 .cirrus.yml  | 63 +++---
 src/tools/ci/ci_macports_packages.sh | 97 
 2 files changed, 122 insertions(+), 38 deletions(-)
 create mode 100755 src/tools/ci/ci_macports_packages.sh

diff --git a/.cirrus.yml b/.cirrus.yml
index d260f15c4e2..e9cfc542cfe 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -429,8 +429,7 @@ task:
 
 CIRRUS_WORKING_DIR: ${HOME}/pgsql/
 CCACHE_DIR: ${HOME}/ccache
-HOMEBREW_CACHE: ${HOME}/homebrew-cache
-PERL5LIB: ${HOME}/perl5/lib/perl5
+MACPORTS_CACHE: ${HOME}/macports-cache
 
 CC: ccache cc
 CXX: ccache c++
@@ -454,55 +453,43 @@ task:
 - mkdir ${HOME}/cores
 - sudo sysctl kern.corefile="${HOME}/cores/core.%P"
 
-  perl_cache:
-folder: ~/perl5
-  cpan_install_script:
-- perl -mIPC::Run -e 1 || cpan -T IPC::Run
-- perl -mIO::Pty -e 1 || cpan -T IO::Pty
-  upload_caches: perl
-
-
-  # XXX: Could we instead install homebrew into a cached directory? The
-  # homebrew installation takes a good bit of time every time, even if the
-  # packages do not need to be downloaded.
-  homebrew_cache:
-folder: $HOMEBREW_CACHE
+  # Use macports, even though homebrew is installed. The installation
+  # of the additional packages we need would take quite a while with
+  # homebrew, even if we cache the downloads. We can't cache all of
+  # homebrew, because it's already large. So we use macports. To cache
+  # the installation we create a .dmg file that we mount if it already
+  # exists.
+  # XXX: The reason for the direct p5.34* references is that we'd need
+  # the large macport tree around to figure out that p5-io-tty is
+  # actually p5.34-io-tty. Using the unversioned name works, but
+  # updates macports every time.
+  macports_cache:
+folder: ${MACPORTS_CACHE}
   setup_additional_packages_script: |
-brew install \
+sh src/tools/ci/ci_macports_packages.sh \
   ccache \
-  icu4c \
-  krb5 \
-  llvm \
+  icu \
+  kerberos5 \
   lz4 \
-  make \
   meson \
   openldap \
   openssl \
-  python \
-  tcl-tk \
+  p5.34-io-tty \
+  p5.34-ipc-run \
+  tcl \
   zstd
-
-brew cleanup -s # to reduce cache size
-  upload_caches: homebrew
+# Make macports install visible for subsequent steps
+echo PATH=/opt/local/sbin/:/opt/local/bin/:$PATH >> $CIRRUS_ENV
+  upload_caches: macports
 
   ccache_cache:
 folder: $CCACHE_DIR
   configure_script: |
-brewpath="/opt/homebrew"
-PKG_CONFIG_PATH="${brewpath}/lib/pkgconfig:${PKG_CONFIG_PATH}"
-
-for pkg in icu4c krb5 openldap openssl zstd ; do
-  pkgpath="${brewpath}/opt/${pkg}"
-  PKG_CONFIG_PATH="${pkgpath}/lib/pkgconfig:${PKG_CONFIG_PATH}"
-  PATH="${pkgpath}/bin:${pkgpath}/sbin:$PATH"
-done
-
-export PKG_CONFIG_PATH PATH
-
+export 

initdb caching during tests

[Andres Freund]

Hi,

We have some issues with CI on macos and windows being too expensive (more on
that soon in a separate email), which reminded me of this thread (with
original title: [1])

I've attached a somewhat cleaned up version of the patch to cache initdb
across runs.  The results are still fairly impressive in my opinion.


One thing I do not like, but don't have a good idea for how to improve, is
that there's a bunch of duplicated logic in pg_regress.c and Cluster.pm. I've
tried to move that into initdb.c itself, but that ends up pretty ugly, because
we need to be a lot more careful about checking whether options are compatible
etc. I've also thought about just putting this into a separate perl script,
but right now we still allow basic regression tests without perl being
available.  So I concluded that for now just having the copies is the best
answer.


Times for running all tests under meson, on my workstation (20 cores / 40
threads):

cassert build -O2:

Before:
real0m44.638s
user7m58.780s
sys 2m48.773s

After:
real0m38.938s
user2m37.615s
sys 2m0.570s


cassert build -O0:

Before:
real1m11.290s
user13m9.817s
sys 2m54.946s

After:
real1m2.959s
user3m5.835s
sys 1m59.887s


non-cassert build:

Before:
real0m34.579s
user5m30.418s
sys 2m40.507s

After:
real0m27.710s
user2m20.644s
sys 1m55.770s


On CI this reduces the test times substantially:
Freebsd   8:51 -> 5:35
Debian w/ asan, autoconf  6:43 -> 4:55
Debian w/ alignmentsan, ubsan 4:02 -> 2:33
macos 5:07 -> 4:29
windows  10:21 -> 9:49

This is ignoring a bit of run-to-run variance, but the trend is obvious enough
that it's not worth worrying about that.

Greetings,

Andres Freund

[1] https://postgr.es/m/20220120021859.3zpsfqn4z7ob7afz%40alap3.anarazel.de
>From 0fd431c277f01284a91999a04368de6b59b6691e Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Thu, 2 Feb 2023 21:51:53 -0800
Subject: [PATCH v3 1/2] Use "template" initdb in tests

Discussion: https://postgr.es/m/20220120021859.3zpsfqn4z7ob7...@alap3.anarazel.de
---
 meson.build  | 30 ++
 .cirrus.yml  |  3 +-
 src/test/perl/PostgreSQL/Test/Cluster.pm | 46 ++-
 src/test/regress/pg_regress.c| 74 ++--
 src/Makefile.global.in   | 52 +
 5 files changed, 161 insertions(+), 44 deletions(-)

diff --git a/meson.build b/meson.build
index 04ea3488522..47429a18c3f 100644
--- a/meson.build
+++ b/meson.build
@@ -3056,8 +3056,10 @@ testport = 4
 test_env = environment()
 
 temp_install_bindir = test_install_location / get_option('bindir')
+test_initdb_template = meson.build_root() / 'tmp_install' / 'initdb-template'
 test_env.set('PG_REGRESS', pg_regress.full_path())
 test_env.set('REGRESS_SHLIB', regress_module.full_path())
+test_env.set('INITDB_TEMPLATE', test_initdb_template)
 
 # Test suites that are not safe by default but can be run if selected
 # by the user via the whitespace-separated list in variable PG_TEST_EXTRA.
@@ -3072,6 +3074,34 @@ if library_path_var != ''
 endif
 
 
+# Create (and remove old) initdb template directory. Tests use that, where
+# possible, to make it cheaper to run tests.
+#
+# Use python to remove the old cached initdb, as we cannot rely on a working
+# 'rm' binary on windows.
+test('initdb_cache',
+ python,
+ args: [
+   '-c', '''
+import shutil
+import sys
+import subprocess
+
+shutil.rmtree(sys.argv[1], ignore_errors=True)
+sp = subprocess.run(sys.argv[2:] + [sys.argv[1]])
+sys.exit(sp.returncode)
+''',
+   test_initdb_template,
+   temp_install_bindir / 'initdb',
+   '-A', 'trust', '-N', '--no-instructions'
+ ],
+ priority: setup_tests_priority - 1,
+ timeout: 300,
+ is_parallel: false,
+ env: test_env,
+ suite: ['setup'])
+
+
 
 ###
 # Test Generation
diff --git a/.cirrus.yml b/.cirrus.yml
index d260f15c4e2..8fce17dff08 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -115,8 +115,9 @@ task:
   test_minimal_script: |
 su postgres <<-EOF
   ulimit -c unlimited
+  meson test $MTEST_ARGS --suite setup
   meson test $MTEST_ARGS --num-processes ${TEST_JOBS} \
-tmp_install cube/regress pg_ctl/001_start_stop
+cube/regress pg_ctl/001_start_stop
 EOF
 
   on_failure:
diff --git a/src/test/perl/PostgreSQL/Test/Cluster.pm b/src/test/perl/PostgreSQL/Test/Cluster.pm
index 5e161dbee60..4d449c35de9 100644
--- a/src/test/perl/PostgreSQL/Test/Cluster.pm
+++ b/src/test/perl/PostgreSQL/Test/Cluster.pm
@@ -522,8 +522,50 @@ sub init
 	mkdir $self->backup_dir;
 	mkdir $self->archive_dir;
 
-	PostgreSQL::Test::Utils::system_or_bail('initdb', '-D', $pgdata, '-A',
-		'trust', '-N', @{ $params{extra} });
+	# If available and if there aren't any parameters, use a 

Re: cataloguing NOT NULL constraints

[Dean Rasheed]

On Sat, 5 Aug 2023 at 18:37, Alvaro Herrera  wrote:
>
> Yeah, something like that.  However, if the child had a NOT NULL
> constraint of its own, then it should not be deleted when the
> PK-on-parent is, but merely marked as no longer inherited.  (This is
> also what happens with a straight NOT NULL constraint.)  I think what
> this means is that at some point during the deletion of the PK we must
> remove the dependency link rather than letting it be followed.  I'm not
> yet sure how to do this.
>

I'm not sure that adding that new dependency was the right thing to
do. I think perhaps this could just be made to work using conislocal
and coninhcount to track whether the child constraint needs to be
deleted, or just updated.

> Anyway, I was at the same time fixing the other problem you reported
> with inheritance (namely, adding a PK ends up with the child column
> being marked NOT NULL but no corresponding constraint).
>
> At some point I wondered if the easy way out wouldn't be to give up on
> the idea that creating a PK causes the child columns to be marked
> not-nullable.  However, IIRC I decided against that because it breaks
> restoring of old dumps, so it wouldn't be acceptable.
>
> To make matters worse: pg_dump creates the PK as
>
>   ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... )
>
> note the ONLY there.  It seems I'm forced to cause the PK to affect
> children even though ONLY is given.  This is undesirable but I don't see
> a way out of that.
>
> It is all a bit of a rat's nest.
>

I wonder if that could be made to work in the same way as inherited
CHECK constraints -- dump the child's inherited NOT NULL constraints,
and then manually update conislocal in pg_constraint.

Regards,
Dean

Re: cataloguing NOT NULL constraints

[Alvaro Herrera]

On 2023-Aug-05, Dean Rasheed wrote:

> Hmm, thinking about this some more, I think this might be the wrong
> approach to fixing the original problem. I think it was probably OK
> that the NOT NULL constraint on the child was marked as inherited, but
> I think what should have happened is that dropping the PRIMARY KEY
> constraint on the parent should have caused the NOT NULL constraint on
> the child to have been deleted (in the same way as it would have been,
> if it had been a NOT NULL constraint on the parent).

Yeah, something like that.  However, if the child had a NOT NULL
constraint of its own, then it should not be deleted when the
PK-on-parent is, but merely marked as no longer inherited.  (This is
also what happens with a straight NOT NULL constraint.)  I think what
this means is that at some point during the deletion of the PK we must
remove the dependency link rather than letting it be followed.  I'm not
yet sure how to do this.

Anyway, I was at the same time fixing the other problem you reported
with inheritance (namely, adding a PK ends up with the child column
being marked NOT NULL but no corresponding constraint).

At some point I wondered if the easy way out wouldn't be to give up on
the idea that creating a PK causes the child columns to be marked
not-nullable.  However, IIRC I decided against that because it breaks
restoring of old dumps, so it wouldn't be acceptable.

To make matters worse: pg_dump creates the PK as 

  ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... )

note the ONLY there.  It seems I'm forced to cause the PK to affect
children even though ONLY is given.  This is undesirable but I don't see
a way out of that.

It is all a bit of a rat's nest.

-- 
Álvaro HerreraBreisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)

Re: cataloguing NOT NULL constraints

[Dean Rasheed]

On Fri, 4 Aug 2023 at 19:10, Alvaro Herrera  wrote:
>
> On 2023-Jul-28, Alvaro Herrera wrote:
>
> > To avoid that, one option would be to make this NN constraint
> > undroppable ...  but I don't see how.  One option might be to add a
> > pg_depend row that links the NOT NULL constraint to its PK constraint.
> > But this will be a strange case that occurs nowhere else, since other
> > NOT NULL constraint don't have such pg_depend rows.  Also, I won't know
> > how pg_dump likes this until I implement it.
>
> I've been completing the implementation for this.  It seems to work
> reasonably okay; pg_dump requires somewhat strange contortions, but they
> are similar to what we do in flagInhTables already, so I don't feel too
> bad about that.
>
> What *is* odd and bothersome is that it also causes a problem dropping
> the child table.
>

Hmm, thinking about this some more, I think this might be the wrong
approach to fixing the original problem. I think it was probably OK
that the NOT NULL constraint on the child was marked as inherited, but
I think what should have happened is that dropping the PRIMARY KEY
constraint on the parent should have caused the NOT NULL constraint on
the child to have been deleted (in the same way as it would have been,
if it had been a NOT NULL constraint on the parent).

Regards,
Dean

Re: brininsert optimization opportunity

[Soumyadeep Chakraborty]

Created an entry for the Sep CF: https://commitfest.postgresql.org/44/4484/

Regards,
Soumyadeep (VMware)

On Sat, Jul 29, 2023 at 9:28 AM Soumyadeep Chakraborty
 wrote:
>
> Attached v4 of the patch, rebased against latest HEAD.
>
> Regards,
> Soumyadeep (VMware)