Re: Fix gcc warning in sync.c (usr/src/backend/storage/sync/sync.c)
[ cc'ing Thomas, whose code this seems to be ]
Kyotaro Horiguchi writes:
> At Sat, 9 Jul 2022 21:53:31 -0300, Ranier Vilela wrote
> in
>> sync.c: In function ¡RememberSyncRequest¢:
>> sync.c:528:10: warning: assignment to ¡PendingFsyncEntry *¢ {aka ¡struct
>> *¢} from incompatible pointer type ¡PendingUnlinkEntry *¢ {aka
>> ¡struct *¢} [-Wincompatible-pointer-types]
>> 528 |entry = (PendingUnlinkEntry *) lfirst(cell);
> Actually, I already see the following line (maybe) at the place instead.
>> PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(cell);
Yeah, I see no line matching that in HEAD either.
However, I do not much like the code at line 528, because its
"PendingUnlinkEntry *entry" is masking an outer variable
"PendingFsyncEntry *entry" from line 513. We should rename
one or both variables to avoid that masking.
regards, tom lane
Re: Fix gcc warning in sync.c (usr/src/backend/storage/sync/sync.c)
At Sat, 9 Jul 2022 21:53:31 -0300, Ranier Vilela wrote in
> sync.c: In function ‘RememberSyncRequest’:
> sync.c:528:10: warning: assignment to ‘PendingFsyncEntry *’ {aka ‘struct
> *’} from incompatible pointer type ‘PendingUnlinkEntry *’ {aka
> ‘struct *’} [-Wincompatible-pointer-types]
> 528 |entry = (PendingUnlinkEntry *) lfirst(cell);
>
> Although the structures are identical, gcc bothers to assign a pointer from
> one to the other.
If the entry were of really PendingSyncEntry, it would need a fix, but
at the same time everyone should see the same warning at their hand.
Actually, I already see the following line (maybe) at the place instead.
529@master,REL14, 508@REL13
> PendingUnlinkEntry *entry = (PendingUnlinkEntry *) lfirst(cell);
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: pgsql: dshash: Add sequential scan support.
On Tue, Jul 5, 2022 at 3:21 PM Thomas Munro wrote: > On Tue, Jul 5, 2022 at 11:25 AM Andres Freund wrote: > > On 2022-07-05 11:20:54 +1200, Thomas Munro wrote: > > > Since there were 6 places with I-hold-no-lock assertions, I shoved the > > > loop into a function so I could do: > > > > > > - Assert(!status->hash_table->find_locked); > > > + assert_no_lock_held_by_me(hash_table); > > > > I am a *bit* wary about the costs of that, even in assert builds - each of > > the > > partition checks in the loop will in turn need to iterate through > > held_lwlocks. But I guess we can also just later weaken them if it turns out > > to be a problem. > > Maybe we should add assertion support for arrays of locks, so we don't > need two levels of loop? Something like the attached? Pushed.
Re: proposal: Allocate work_mem From Pool
On Sun, Jul 10, 2022 at 08:45:38PM -0700, Joseph D Wagner wrote: > However, that's risky because it's 3GB per operation, not per > query/connection; it could easily spiral out of control. This is a well-known deficiency. I suggest to dig up the old threads to look into. It's also useful to include links to the prior discussion. > I think it would be better if work_mem was allocated from a pool of memory I think this has been proposed before, and the issue/objection with this idea is probably that query plans will be inconsistent, and end up being sub-optimal. work_mem is considered at planning time, but I think you only consider its application execution. A query that was planned with the configured work_mem but can't obtain the expected amount at execution time might perform poorly. Maybe it should be replanned with lower work_mem, but that would lose the arms-length relationship between the planner-executor. Should an expensive query wait a bit to try to get more work_mem? What do you do if 3 expensive queries are all waiting ? -- Justin
Re: [PATCH] Optimize json_lex_string by batching character copying
On Fri, Jul 8, 2022 at 3:06 PM John Naylor wrote: > > I've pushed 0001 (although the email seems to have been swallowed > again), and pending additional comments on 0002 and 0003 I'll squash > and push those next week. This is done. > 0004 needs some thought on integrating with > symbols we discover during configure. Still needs thought. -- John Naylor EDB: http://www.enterprisedb.com
wal write of large object
Hi, hackers I'm eager to dive into how to write wal for large object. In the code path: heap_insert -> heap_prepare_insert -> heap_toast_insert_or_update -> toast_save_datum -> heap_insert I find heap_insert is called back. 1. At heaptup = heap_prepare_insert(relation, tup, xid, cid, options), the comment says tup is untoasted data, but it could come from toast_save_datum, still untoasted ? 2. At toast_save_datum while loop, we know heap_insert is called with every chunk data, so every chunk data is written to WAL by XLogInsert() , right ? Although it may make WAL big. (It is still the case regarding to blob, cblob ?) 3. When heap_insert is called firstly and heap_prepare_insert returns, what does heaptup mean for large object which has been chunked and written by toast_save_datum ? Any articles about this aspect would be appreciated.
Re: Fast COPY FROM based on batch insert
On 11/7/2022 04:12, Ian Barwick wrote: On 09/07/2022 00:09, Andrey Lepikhov wrote: On 8/7/2022 05:12, Ian Barwick wrote: ERROR: bind message supplies 0 parameters, but prepared statement "pgsql_fdw_prep_178" requires 6 CONTEXT: remote SQL command: INSERT INTO public.foo_part_1(t, v1, v2, v3, v4, v5) VALUES ($1, $2, $3, $4, $5, $6) COPY foo, line 88160 Thanks, I got it. MultiInsertBuffer are created on the first non-zero flush of tuples into the partition and isn't deleted from the buffers list until the end of COPY. And on a subsequent flush in the case of empty buffer we catch the error. Your fix is correct, but I want to propose slightly different change (see in attachment). LGTM. New version (with aforementioned changes) is attached. -- regards, Andrey Lepikhov Postgres ProfessionalFrom 976560f2ad406adba1aaf58a188b44302855ee12 Mon Sep 17 00:00:00 2001 From: "Andrey V. Lepikhov" Date: Fri, 4 Jun 2021 13:21:43 +0500 Subject: [PATCH] Implementation of a Bulk COPY FROM operation into foreign table. --- .../postgres_fdw/expected/postgres_fdw.out| 45 +++- contrib/postgres_fdw/sql/postgres_fdw.sql | 47 src/backend/commands/copyfrom.c | 211 -- src/backend/executor/execMain.c | 45 src/backend/executor/execPartition.c | 8 + src/include/commands/copyfrom_internal.h | 10 - src/include/executor/executor.h | 1 + src/include/nodes/execnodes.h | 5 +- 8 files changed, 238 insertions(+), 134 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 44457f930c..aced9a6428 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -8435,6 +8435,7 @@ drop table loct2; -- === -- test COPY FROM -- === +alter server loopback options (add batch_size '2'); create table loc2 (f1 int, f2 text); alter table loc2 set (autovacuum_enabled = 'false'); create foreign table rem2 (f1 int, f2 text) server loopback options(table_name 'loc2'); @@ -8457,7 +8458,7 @@ copy rem2 from stdin; -- ERROR ERROR: new row for relation "loc2" violates check constraint "loc2_f1positive" DETAIL: Failing row contains (-1, xyzzy). CONTEXT: remote SQL command: INSERT INTO public.loc2(f1, f2) VALUES ($1, $2) -COPY rem2, line 1: "-1 xyzzy" +COPY rem2, line 2 select * from rem2; f1 | f2 +- @@ -8468,6 +8469,19 @@ select * from rem2; alter foreign table rem2 drop constraint rem2_f1positive; alter table loc2 drop constraint loc2_f1positive; delete from rem2; +create table foo (a int) partition by list (a); +create table foo1 (like foo); +create foreign table ffoo1 partition of foo for values in (1) + server loopback options (table_name 'foo1'); +create table foo2 (like foo); +create foreign table ffoo2 partition of foo for values in (2) + server loopback options (table_name 'foo2'); +create function print_new_row() returns trigger language plpgsql as $$ + begin raise notice '%', new; return new; end; $$; +create trigger ffoo1_br_trig before insert on ffoo1 + for each row execute function print_new_row(); +copy foo from stdin; +NOTICE: (1) -- Test local triggers create trigger trig_stmt_before before insert on rem2 for each statement execute procedure trigger_func(); @@ -8576,6 +8590,34 @@ drop trigger rem2_trig_row_before on rem2; drop trigger rem2_trig_row_after on rem2; drop trigger loc2_trig_row_before_insert on loc2; delete from rem2; +alter table loc2 drop column f1; +alter table loc2 drop column f2; +copy rem2 from stdin; +ERROR: column "f1" of relation "loc2" does not exist +CONTEXT: remote SQL command: INSERT INTO public.loc2(f1, f2) VALUES ($1, $2), ($3, $4) +COPY rem2, line 3 +alter table loc2 add column f1 int; +alter table loc2 add column f2 int; +select * from rem2; + f1 | f2 ++ +(0 rows) + +-- dropped columns locally and on the foreign server +alter table rem2 drop column f1; +alter table rem2 drop column f2; +copy rem2 from stdin; +select * from rem2; +-- +(2 rows) + +alter table loc2 drop column f1; +alter table loc2 drop column f2; +copy rem2 from stdin; +select * from rem2; +-- +(4 rows) + -- test COPY FROM with foreign table created in the same transaction create table loc3 (f1 int, f2 text); begin; @@ -8592,6 +8634,7 @@ select * from rem3; drop foreign table rem3; drop table loc3; +alter server loopback options (drop batch_size); -- === -- test for TRUNCATE -- === diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql index 92d1212027..5c047ce8ee 100644 ---
proposal: Allocate work_mem From Pool
I'm new here, so forgive me if this is a bad idea or my lack of knowledge on how to optimize PostgreSQL. I find PostgreSQL to be great with a large number of small transactions, which covers most use cases. However, my experience has not been so great on the opposite end -- a small number of large transactions, i.e. Big Data. I had to increase work_mem to 3GB to stop my queries from spilling to disk. However, that's risky because it's 3GB per operation, not per query/connection; it could easily spiral out of control. I think it would be better if work_mem was allocated from a pool of memory as need and returned to the pool when no longer needed. The pool could optionally be allocated from huge pages. It would allow large and mixed workloads the flexibility of grabbing more memory as needed without spilling to disk while simultaneously being more deterministic about the maximum that will be used. Thoughts? Thank you for your time. Joseph D. Wagner My specifics: -64 GB box -16 GB shared buffer, although queries only using about 12 GB of that -16 GB effective cache -2-3 GB used by OS and apps -the rest is available for Postgresql queries/connections/whatever as needed
Re: Handle infinite recursion in logical replication setup
Here are my review comments for the v30* patches:
v30-0001
1.1
I was wondering if it is better to implement a new defGetOrigin method
now instead of just using the defGetString to process the 'origin',
since you may need to do that in future anyway if the 'origin' name is
planned to become specifiable by the user. OTOH maybe you prefer to
change this code later when the time comes. I am not sure what way is
best.
~~~
1.2. src/include/replication/walreceiver.h
@@ -183,6 +183,8 @@ typedef struct
bool streaming; /* Streaming of large transactions */
bool twophase; /* Streaming of two-phase transactions at
* prepare time */
+ char*origin; /* Only publish data originating from the
+ * specified origin */
} logical;
} proto;
} WalRcvStreamOptions;
Should the new comments be aligned with the other ones?
v30-0002
2.1 doc/src/sgml/ref/alter_subscription.sgml
+
+ Refer for the usage of
+ force for copy_data parameter
+ and its interaction with the origin parameter.
IMO it's better to say "Refer to the Notes" or "Refer to CREATE
SUBSCRIPTION Notes" instead of just "Refer Notes"
~~~
2.2 doc/src/sgml/ref/create_subscription.sgml
2.2.a
+
+ Refer for the usage of
+ force for copy_data parameter
+ and its interaction with the origin parameter.
+
IMO it's better to say "Refer to the Notes" (same as other xref on
this page) instead of "Refer Notes"
2.2.b
@@ -316,6 +324,11 @@ CREATE SUBSCRIPTION subscription_nameany.
+
+ Refer for the usage of
+ force for copy_data parameter
+ and its interaction with the origin parameter.
+
Ditto
~~~
2.3 src/backend/commands/subscriptioncmds.c - DefGetCopyData
+/*
+ * Validate the value specified for copy_data parameter.
+ */
+static CopyData
+DefGetCopyData(DefElem *def)
~~~
2.4
+ /*
+ * If no parameter given, assume "true" is meant.
+ */
Please modify the comment to match the recent push [1].
~~~
2.5 src/test/subscription/t/032_localonly.pl
2.5.a
+# Check Alter subscription ... refresh publication when there is a new
+# table that is subscribing data from a different publication
+$node_A->safe_psql('postgres', "CREATE TABLE tab_full1 (a int PRIMARY KEY)");
+$node_B->safe_psql('postgres', "CREATE TABLE tab_full1 (a int PRIMARY KEY)");
Unfortunately, I think tab_full1 is a terrible table name because in
my screen font the 'l' and the '1' look exactly the same so it just
looks like a typo. Maybe change it to "tab_new" or something?
2.5b
What exactly is the purpose of "full" in all these test table names?
AFAIK "full" is just some name baggage inherited from completely
different tests which were doing full versus partial table
replication. I'm not sure it is relevant here.
v30-0002
No comments.
--
[1]
https://github.com/postgres/postgres/commit/8445f5a21d40b969673ca03918c74b4fbc882bf4
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Add function to return backup_label and tablespace_map
At Fri, 8 Jul 2022 01:43:49 +0900, Fujii Masao
wrote in
> finishes. For example, this function allows us to take a backup using
> the following psql script file.
>
> --
> SELECT * FROM pg_backup_start('test');
> \! cp -a $PGDATA /backup
> SELECT * FROM pg_backup_stop();
>
> \pset tuples_only on
> \pset format unaligned
>
> \o /backup/data/backup_label
> SELECT labelfile FROM pg_backup_label();
>
> \o /backup/data/tablespace_map
> SELECT spcmapfile FROM pg_backup_label();
> --
As David mentioned, we can do the same thing now by using \gset, when
we want to save the files on the client side. (File copy is done on
the server side by the steps, though.)
Thinking about another scenario of generating those files server-side
(this is safer than the client-side method regarding to
line-separators and the pset settings, I think). We can do that by
using admingpack instead, with simpler steps.
SELECT lsn, labelfile, spcmapfile
pg_file_write('/tmp/backup_label', labelfile, false),
pg_file_write('/tmp/tablespace_map', spcmapfile, false)
FROM pg_backup_stop();
However, if pg_file_write() fails, the data are gone. But \gset also
works here.
select pg_backup_start('s1');
SELECT * FROM pg_backup_stop() \gset
SELECT pg_file_write('/tmp/backup_label', :'labelfile', false);
SELECT pg_file_write('/tmp/tablespace_map', :'spcmapfile', false);
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: defGetBoolean - Fix comment
On Mon, Jul 11, 2022 at 12:09 PM Michael Paquier wrote: > > On Thu, Jul 07, 2022 at 09:54:24AM +0900, Michael Paquier wrote: > > Still, I think that your adjustment is right, as the check is, indeed, > > on the DefElem's value*. Or you could just say "If no value given". > > Peter, I have applied something using your original wording. This is > a minor issue, but I did not see my suggestion as being better than > yours. Thanks for pushing it. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: defGetBoolean - Fix comment
On Thu, Jul 07, 2022 at 09:54:24AM +0900, Michael Paquier wrote: > Still, I think that your adjustment is right, as the check is, indeed, > on the DefElem's value*. Or you could just say "If no value given". Peter, I have applied something using your original wording. This is a minor issue, but I did not see my suggestion as being better than yours. -- Michael signature.asc Description: PGP signature
Re: Extending outfuncs support to utility statements
Hi, Now we are ready to have debug_print_raw_parse (or something like that)? Pgpool-II has been importing and using PostgreSQL's raw parser for years. I think it would be great for PostgreSQL and Pgpool-II developers to have such a feature. Best reagards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp
Re: pg15b2: large objects lost on upgrade
On Fri, Jul 08, 2022 at 10:44:07AM -0400, Robert Haas wrote: > Thanks for checking over the reasoning, and the kind words in general. Thanks for fixing the main issue. > I just committed Justin's fix for the bug, without fixing the fact > that the new cluster's original pg_largeobject files will be left > orphaned afterward. That's a relatively minor problem by comparison, > and it seemed best to me not to wait too long to get the main issue > addressed. Hmm. That would mean that the more LOs a cluster has, the more bloat there will be in the new cluster once the upgrade is done. That could be quite a few gigs worth of data laying around depending on the data inserted in the source cluster, and we don't have a way to know which files to remove post-upgrade, do we? -- Michael signature.asc Description: PGP signature
Re: Bump MIN_WINNT to 0x0600 (Vista) as minimal runtime in 16~
On Thu, Jul 07, 2022 at 01:56:39PM +0900, Michael Paquier wrote: > And I have applied that, after noticing that the MinGW was complaining > because _WIN32_WINNT was not getting set like previously and removing > _WIN32_WINNT as there is no need for it anymore. The CI has reported > green for all my tests, so I am rather confident to not have messed up > something. Now, let's see what the buildfarm members tell. This > should take a couple of hours.. Since this has been applied, all the Windows members have reported a green state except for jacana and bowerbird. Based on their environment, I would not expect any issues though I may be wrong. Andrew, is something happening on those environments? Is 495ed0e causing problems? -- Michael signature.asc Description: PGP signature
Re: Allow file inclusion in pg_hba and pg_ident files
On Fri, Jul 08, 2022 at 02:57:21PM +0800, Julien Rouhaud wrote: My apologies for the late reply. > I don't have an extensive knowledge of all the user facing error messages, but > after a quick grep I see multiple usage of OID, PID, GIN and other defined > acronyms. I also see multiple occurrences of "only heap AM is supported", > while AM isn't even a defined acronym. A lot depends on the context of the code, it seems. > It doesn't seem that my proposal would be inconsistent with existing messages > and will help to reduce the message length, but if you prefer to keep the full > name I'm fine with it. Those should be very rare and specialized errors > anyway. So you mean to use "HBA file" instead of pg_hba.conf and "authentication file" when it can be either one of an HBA file or a mapping file? That would be okay by me. We would have a full cycle to tune them depending on the feedback we'd get afterwards. > While on the bikeshedding part, are you ok with the proposed keywords (include > and include_dir), behaving exactly like for postgresql.conf, and to also add > include_if_exists, so that we have the exact same possibilities with > postgresql.conf, pg_hba.conf and pg_ident.conf? Okay, agreed for consistency. With include_dir being careful about the ordering of the entries and ignoring anything else than a .conf file (that's something you mentioned already upthread). -- Michael signature.asc Description: PGP signature
Re: Fast COPY FROM based on batch insert
On 09/07/2022 00:09, Andrey Lepikhov wrote: On 8/7/2022 05:12, Ian Barwick wrote: ERROR: bind message supplies 0 parameters, but prepared statement "pgsql_fdw_prep_178" requires 6 CONTEXT: remote SQL command: INSERT INTO public.foo_part_1(t, v1, v2, v3, v4, v5) VALUES ($1, $2, $3, $4, $5, $6) COPY foo, line 88160 Thanks, I got it. MultiInsertBuffer are created on the first non-zero flush of tuples into the partition and isn't deleted from the buffers list until the end of COPY. And on a subsequent flush in the case of empty buffer we catch the error. Your fix is correct, but I want to propose slightly different change (see in attachment). LGTM. Regards Ian Barwick -- https://www.enterprisedb.com/
Re: PG 15 (and to a smaller degree 14) regression due to ExprEvalStep size
Hi, On 2022-07-08 17:05:49 -0400, Andrew Dunstan wrote: > Actually, it's not the same name: JsonCoercionsState vs > JsonCoercionState. But I agree that it's a subtle enough difference that > we should use something more obvious. Maybe JsonCoercionStates instead > of JsonCoercionsState? The plural at the end would be harder to miss. Given that it's a one-off use struct, why name it? Then we don't have to figure out a name we never use. I also still would like to understand why we need pre-allocated space for all these types. How could multiple datums be coerced in an interleaved manner? And if that's possible, why can't multiple datums of the same type be coerced at the same time? Greetings, Andres Freund
Re: Extending outfuncs support to utility statements
Andres Freund writes: > On 2022-07-10 19:12:52 -0400, Tom Lane wrote: >> They're not so much "cold" as "dead", so I don't see the point >> of having them at all. If we ever start allowing utility commands >> (besides NOTIFY) in stored rules, we'd need readfuncs support then >> ... but at least in the short run I don't see that happening. > It would allow us to test utility outfuncs as part of the > WRITE_READ_PARSE_PLAN_TREES check. Not that that's worth very much. Especially now that those are all auto-generated anyway. > I guess it could be a minor help in making a few more utility commands benefit > from paralellism? Again, once we have an actual use-case, enabling that code will be fine by me. But we don't yet. regards, tom lane
Re: Extending outfuncs support to utility statements
Hi, On 2022-07-10 19:12:52 -0400, Tom Lane wrote: > Andres Freund writes: > > On 2022-07-09 18:20:26 -0400, Tom Lane wrote: > >> For my taste, the circa 20K growth in outfuncs.o is an okay > >> price for being able to inspect utility statements more easily. > >> However, I'm less thrilled with the 30K growth in readfuncs.o, > >> because I can't see that we'd get any direct benefit from that. > >> So I think a realistic proposal is to enable outfuncs support > >> but keep readfuncs disabled. > > > Another approach could be to mark those paths as "cold", so they are placed > > further away, reducing / removing potential overhead due to higher iTLB > > misses > > etc. 30K of disk space isn't worth worrying about. > > They're not so much "cold" as "dead", so I don't see the point > of having them at all. If we ever start allowing utility commands > (besides NOTIFY) in stored rules, we'd need readfuncs support then > ... but at least in the short run I don't see that happening. It would allow us to test utility outfuncs as part of the WRITE_READ_PARSE_PLAN_TREES check. Not that that's worth very much. I guess it could be a minor help in making a few more utility commands benefit from paralellism? Anyway, as mentioned earlier, I'm perfectly fine not supporting readfuns for utility statements for now. Greetings, Andres Freund
Re: AIX support - alignment issues
On Mon, Jul 11, 2022 at 11:38 AM Tom Lane wrote: > WFM. I also wonder if in > > + PostgreSQL can be expected to work on current > + versions of these operating systems: Linux (all recent distributions), > Windows, > + FreeBSD, OpenBSD, NetBSD, DragonFlyBSD, macOS, AIX, Solaris, and illumos. > > we could drop "(all recent distributions)", figuring that "current > versions" covers that already. Other than that niggle, this > looks good to me. Yeah. I wasn't too sure if that was mostly about "recent" or mostly about "all distributions" but it wasn't doing much. Thanks, pushed.
Re: Making Vars outer-join aware
Zhihong Yu writes:
> In remove_unneeded_nulling_relids():
> + if (removable_relids == NULL)
> Why is bms_is_empty() not used in the above check ?
We initialized that to NULL just a few lines above, and then did
nothing to it other than perhaps bms_add_member, so it's impossible
for it to be empty-and-yet-not-NULL.
> +typedef struct reduce_outer_joins_partial_state
> Since there are already reduce_outer_joins_pass1_state
> and reduce_outer_joins_pass2_state, a comment
> above reduce_outer_joins_partial_state would help other people follow its
> purpose.
We generally document these sorts of structs in the using code,
not at the struct declaration.
> + if (j->rtindex)
> + {
> + if (j->jointype == JOIN_INNER)
> + {
> + if (include_inner_joins)
> + result = bms_add_member(result, j->rtindex);
> + }
> + else
> + {
> + if (include_outer_joins)
> Since there are other join types beside JOIN_INNER, should there be an
> assertion in the else block?
Like what? We don't particularly care what the join type is here,
as long as it's not INNER. In any case there is plenty of nearby
code checking for unsupported join types.
regards, tom lane
Re: AIX support - alignment issues
Thomas Munro writes: > OK, I word-smothe thusly: > + and PA-RISC, including > + big-endian, little-endian, 32-bit, and 64-bit variants where applicable. WFM. I also wonder if in + PostgreSQL can be expected to work on current + versions of these operating systems: Linux (all recent distributions), Windows, + FreeBSD, OpenBSD, NetBSD, DragonFlyBSD, macOS, AIX, Solaris, and illumos. we could drop "(all recent distributions)", figuring that "current versions" covers that already. Other than that niggle, this looks good to me. regards, tom lane
Re: AIX support - alignment issues
On Fri, Jul 8, 2022 at 4:24 PM Tom Lane wrote:
> Thomas Munro writes:
> > * The documented list mentions some in different endiannesses and word
> > sizes explicitly but not others; I think it'd be tidier to list the
> > main architecture names and then tack on a "big and little endian, 32
> > and 64 bit" sentence.
>
> As phrased, this seems to be saying that we can do both
> endiannesses on any of the supported arches, which is a little
> weird considering that most of them are single-endianness. It's
> not a big deal, but maybe a tad more word-smithing there would
> help?
OK, I word-smothe thusly:
+ and PA-RISC, including
+ big-endian, little-endian, 32-bit, and 64-bit variants where applicable.
I also realised that we should list a couple more OSes (we know they
work, they are automatically tested). Then I wondered why we bother
to state a Windows version here. For consistency, we could list the
minimum Linux kernel, and so on for every other OS, but that's silly
for such brief and general documentation. So I propose that we just
say "current versions of ..." and remove the bit about Windows 10.
From 395e31b5ddb142230b9ffaf60af1f56fea46383d Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Fri, 8 Jul 2022 10:19:32 +1200
Subject: [PATCH v2] Tidy up claimed supported CPUs and OSes.
* Remove arbitrary mention of certain endianness and bitness variants;
it's enough to say that applicable variants are expected to work.
* List RISC-V (known to work, being tested).
* List SuperH and M88K (code exists, unknown status, like M68K).
* De-list VAX and remove code (known not to work).
* Remove stray traces of Alpha (support was removed years ago).
* List illumos, DragonFlyBSD (known to work, being tested).
* No need to single Windows out by listing a specific version, when we
don't do that for other OSes; it's enough to say that we support
current versions of the listed OSes (when PG16 ships, that'll be
Windows 10+).
Reviewed-by: Tom Lane
Reviewed-by: Greg Stark
Discussion: https://postgr.es/m/CA%2BhUKGKk7NZO1UnJM0PyixcZPpCGqjBXW_0bzFZpJBGAf84XKg%40mail.gmail.com
---
contrib/pgcrypto/crypt-blowfish.c | 2 +-
doc/src/sgml/installation.sgml| 13 +++--
src/include/storage/s_lock.h | 30 --
3 files changed, 8 insertions(+), 37 deletions(-)
diff --git a/contrib/pgcrypto/crypt-blowfish.c b/contrib/pgcrypto/crypt-blowfish.c
index a663852ccf..1264eccb3f 100644
--- a/contrib/pgcrypto/crypt-blowfish.c
+++ b/contrib/pgcrypto/crypt-blowfish.c
@@ -41,7 +41,7 @@
#ifdef __i386__
#define BF_ASM0 /* 1 */
#define BF_SCALE 1
-#elif defined(__x86_64__) || defined(__alpha__) || defined(__hppa__)
+#elif defined(__x86_64__) || defined(__hppa__)
#define BF_ASM0
#define BF_SCALE 1
#else
diff --git a/doc/src/sgml/installation.sgml b/doc/src/sgml/installation.sgml
index 1a1343a008..09cd9d8b9c 100644
--- a/doc/src/sgml/installation.sgml
+++ b/doc/src/sgml/installation.sgml
@@ -2125,18 +2125,19 @@ export MANPATH
In general, PostgreSQL can be expected to work on
- these CPU architectures: x86, x86_64, PowerPC,
- PowerPC 64, S/390, S/390x, Sparc, Sparc 64, ARM, MIPS, MIPSEL,
- and PA-RISC. Code support exists for M68K, M32R, and VAX, but these
+ these CPU architectures: x86, PowerPC, S/390, Sparc, ARM, MIPS, RISC-V,
+ and PA-RISC, including
+ big-endian, little-endian, 32-bit, and 64-bit variants where applicable.
+ Code support exists for M68K, M88K, M32R, and SuperH, but these
architectures are not known to have been tested recently. It is often
possible to build on an unsupported CPU type by configuring with
--disable-spinlocks, but performance will be poor.
- PostgreSQL can be expected to work on these operating
- systems: Linux (all recent distributions), Windows (10 and later),
- FreeBSD, OpenBSD, NetBSD, macOS, AIX, and Solaris.
+ PostgreSQL can be expected to work on current
+ versions of these operating systems: Linux (all recent distributions), Windows,
+ FreeBSD, OpenBSD, NetBSD, DragonFlyBSD, macOS, AIX, Solaris, and illumos.
Other Unix-like systems may also work but are not currently
being tested. In most cases, all CPU architectures supported by
a given operating system will work. Look in
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index c4a19b2f43..1f5394ef7f 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -548,36 +548,6 @@ tas(volatile slock_t *lock)
#endif /* __m88k__ */
-/*
- * VAXen -- even multiprocessor ones
- * (thanks to Tom Ivar Helbekkmo)
- */
-#if defined(__vax__)
-#define HAS_TEST_AND_SET
-
-typedef unsigned char slock_t;
-
-#define TAS(lock) tas(lock)
-
-static __inline__ int
-tas(volatile slock_t *lock)
-{
- register int _res;
-
- __asm__ __volatile__(
- " movl $1, %0 \n"
- " bbssi $0, (%2), 1f \n"
- " clrl %0\n"
- "1: \n"
-: "="(_res), "+m"(*lock)
-: "r"(lock)
-:
Re: Extending outfuncs support to utility statements
Andres Freund writes: > On 2022-07-09 18:20:26 -0400, Tom Lane wrote: >> For my taste, the circa 20K growth in outfuncs.o is an okay >> price for being able to inspect utility statements more easily. >> However, I'm less thrilled with the 30K growth in readfuncs.o, >> because I can't see that we'd get any direct benefit from that. >> So I think a realistic proposal is to enable outfuncs support >> but keep readfuncs disabled. > Another approach could be to mark those paths as "cold", so they are placed > further away, reducing / removing potential overhead due to higher iTLB misses > etc. 30K of disk space isn't worth worrying about. They're not so much "cold" as "dead", so I don't see the point of having them at all. If we ever start allowing utility commands (besides NOTIFY) in stored rules, we'd need readfuncs support then ... but at least in the short run I don't see that happening. regards, tom lane
Re: automatically generating node support functions
Andres Freund writes: > I was just rebasing meson ontop of this and was wondering whether the input > filenames were in a particular order: That annoyed me too. I think it's sensible to list the "main" input files first, but I'd put them in our traditional pipeline order: > nodes/nodes.h \ > nodes/primnodes.h \ > nodes/parsenodes.h \ > nodes/pathnodes.h \ > nodes/plannodes.h \ > nodes/execnodes.h \ The rest could probably be alphabetical. I was also wondering if all of them really need to be read at all --- I'm unclear on what access/sdir.h is contributing, for example. regards, tom lane
Re: System catalog documentation chapter
On Sat, Jul 9, 2022 at 5:32 AM Bruce Momjian wrote: > ... > Attached is a patch to accomplish this. Its output can be seen here: > > https://momjian.us/tmp/pgsql/internals.html > That output looks good to me. -- Kind Regards, Peter Smith. Fujitsu Australia
Re: pg_waldump got an error with waldump file generated by initdb
On Sun, Jul 10, 2022 at 02:39:00PM -0700, Andres Freund wrote: > On 2022-07-10 21:51:04 +0900, Dong Wook Lee wrote: > > I don't know if this is an error. > > when I do ./initdb -D ../data and execute pg_waldump like this, In the last > > part I got an error. > > > > ``` > > ./pg_waldump ../data/pg_wal/00010001 > > ``` > > > > pg_waldump: error: error in WAL record at 0/140: invalid record length > > at 0/1499A08: wanted 24, got 0 > > > > my environment is `16devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu > > 9.4.0-1ubuntu1~20.04.1) 9.4.0, 64-bit` > > Is this normal? > > Yes, that's likely normal - i.e. pg_waldump has reached the point at which the > WAL ends. It's the issue that's fixed by this patch. 38/2490 Make message at end-of-recovery less scary Kyotaro Horiguchi https://commitfest.postgresql.org/38/2490/
Re: automatically generating node support functions
Hi, On 2022-07-09 16:37:22 +0200, Peter Eisentraut wrote: > On 08.07.22 22:03, Tom Lane wrote: > > I think this is ready to go (don't forget the catversion bump). > > This is done now, after a brief vpath-shaped scare from the buildfarm > earlier today. I was just rebasing meson ontop of this and was wondering whether the input filenames were in a particular order: node_headers = \ nodes/nodes.h \ nodes/execnodes.h \ nodes/plannodes.h \ nodes/primnodes.h \ nodes/pathnodes.h \ nodes/extensible.h \ nodes/parsenodes.h \ nodes/replnodes.h \ nodes/value.h \ commands/trigger.h \ commands/event_trigger.h \ foreign/fdwapi.h \ access/amapi.h \ access/tableam.h \ access/tsmapi.h \ utils/rel.h \ nodes/supportnodes.h \ executor/tuptable.h \ nodes/lockoptions.h \ access/sdir.h Can we either order them alphabetically or add a comment explaining the order? - Andres
Re: Extending outfuncs support to utility statements
Hi, On 2022-07-09 18:20:26 -0400, Tom Lane wrote: > We've long avoided building I/O support for utility-statement node > types, mainly because it didn't seem worth the trouble to write and > maintain such code by hand. Now that the automatic node-support-code > generation patch is in, that argument is gone, and it's just a matter > of whether the benefits are worth the backend code bloat. I can > see two benefits worth considering: > > * Seems like having such support would be pretty useful for > debugging. Agreed. > So I looked into how much code are we talking about. On my > RHEL8 x86_64 machine, the code sizes for outfuncs/readfuncs > as of HEAD are > > $ size outfuncs.o readfuncs.o >textdata bss dec hex filename > 117173 0 0 117173 1c9b5 outfuncs.o > 64540 0 0 64540fc1c readfuncs.o > > If we just open the floodgates and enable both outfuncs and > readfuncs support for all *Stmt nodes (plus some node types > that thereby become dumpable, like AlterTableCmd), then > this becomes > > $ size outfuncs.o readfuncs.o >textdata bss dec hex filename > 139503 0 0 139503 220ef outfuncs.o > 95562 0 0 95562 1754a readfuncs.o > > For my taste, the circa 20K growth in outfuncs.o is an okay > price for being able to inspect utility statements more easily. > However, I'm less thrilled with the 30K growth in readfuncs.o, > because I can't see that we'd get any direct benefit from that. > So I think a realistic proposal is to enable outfuncs support > but keep readfuncs disabled. Another approach could be to mark those paths as "cold", so they are placed further away, reducing / removing potential overhead due to higher iTLB misses etc. 30K of disk space isn't worth worrying about. Don't really have an opinion on this. Greetings, Andres Freund
Re: pg_waldump got an error with waldump file generated by initdb
Hi, On 2022-07-10 21:51:04 +0900, Dong Wook Lee wrote: > I don't know if this is an error. > when I do ./initdb -D ../data and execute pg_waldump like this, In the last > part I got an error. > > ``` > ./pg_waldump ../data/pg_wal/00010001 > ``` > > pg_waldump: error: error in WAL record at 0/140: invalid record length at > 0/1499A08: wanted 24, got 0 > > my environment is `16devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu > 9.4.0-1ubuntu1~20.04.1) 9.4.0, 64-bit` > Is this normal? Yes, that's likely normal - i.e. pg_waldump has reached the point at which the WAL ends. Greetings, Andres Freund
Re: Making Vars outer-join aware
On Sun, Jul 10, 2022 at 12:39 PM Tom Lane wrote:
> Here's v2 of this patch series. It's functionally identical to v1,
> but I've rebased it over the recent auto-node-support-generation
> changes, and also extracted a few separable bits in hopes of making
> the main planner patch smaller. (It's still pretty durn large,
> unfortunately.) Unlike the original submission, each step will
> compile on its own, though the intermediate states mostly don't
> pass all regression tests.
>
> regards, tom lane
>
> Hi,
For v2-0004-cope-with-nullability-in-planner.patch.
In remove_unneeded_nulling_relids():
+ if (removable_relids == NULL)
Why is bms_is_empty() not used in the above check ?
Earlier there is `if (bms_is_empty(old_nulling_relids))`
+typedef struct reduce_outer_joins_partial_state
Since there are already reduce_outer_joins_pass1_state
and reduce_outer_joins_pass2_state, a comment
above reduce_outer_joins_partial_state would help other people follow its
purpose.
+ if (j->rtindex)
+ {
+ if (j->jointype == JOIN_INNER)
+ {
+ if (include_inner_joins)
+ result = bms_add_member(result, j->rtindex);
+ }
+ else
+ {
+ if (include_outer_joins)
Since there are other join types beside JOIN_INNER, should there be an
assertion in the else block ? e.g. jointype wouldn't be JOIN_UNIQUE_INNER.
Cheers
Re: Cleaning up historical portability baggage
(Reading the patch it seems both those points are already addressed)
Re: Two successive tabs in test case are causing syntax error in psql
On 2022-Jul-08, Tom Lane wrote: > The usual recommendation for pasting text into psql when it contains > tabs is to start psql with the -n switch to disable tab completion. "Bracketed paste" also solves this problem. To enable this feature, just edit your $HOME/.inputrc file to have the line set enable-bracketed-paste on (then restart psql) which will cause the text passed to be used literally, so the tabs won't invoke tab-completion. There are other side-effects: if you paste a multi-command string, the whole string is added as a single entry in the history rather than being separate entries. I find this extremely useful; there are also claims of this being more secure. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
Re: Cleaning up historical portability baggage
On Sat, 9 Jul 2022 at 21:46, Thomas Munro wrote: > > Hello, > > I wonder how much dead code for ancient operating systems we could now > drop. > 0002-Remove-dead-getrusage-replacement-code.patch I thought the getrusage replacement code was for Windows. Does getrusage on Windows actually do anything useful? More generally I think there is a question about whether some of these things are "supported" in only a minimal way to satisfy standards but maybe not in a way that we actually want to use. Getrusage might exist on Windows but not actually report the metrics we need, reentrant library functions may be implemented by simply locking instead of actually avoiding static storage, etc. -- greg
Re: Cleaning up historical portability baggage
I wrote: > Having said that, I'll be happy to try out this patch series on > that platform and see if it burps. HEAD + patches 0001-0006 seems fine on prairiedog's host. Builds clean (or as clean as HEAD does anyway), passes make check. I did not trouble with check-world. (I haven't actually read the patches, so this isn't a review, just a quick smoke-test.) regards, tom lane
Re: Compilation issue on Solaris.
Thomas Munro writes: > Something bothers me about adding yet more clutter to every compile > line for the rest of time to solve a problem that exists only for > unpatched systems, and also that it's not even really a Solaris thing, > it's a C11 thing. I tend to agree with this standpoint: if it's only a warning, and it only appears in a small range of not-up-to-date Solaris builds, then a reasonable approach is "update your system if you don't want to see the warning". A positive argument for doing nothing is that there's room to worry whether -D__STDC_WANT_LIB_EXT1__ might have any side-effects we *don't* want. regards, tom lane
pg_waldump got an error with waldump file generated by initdb
Hi, hackers I don't know if this is an error. when I do ./initdb -D ../data and execute pg_waldump like this, In the last part I got an error. ``` ./pg_waldump ../data/pg_wal/00010001 ``` pg_waldump: error: error in WAL record at 0/140: invalid record length at 0/1499A08: wanted 24, got 0 my environment is `16devel on x86_64-pc-linux-gnu, compiled by gcc (Ubuntu 9.4.0-1ubuntu1~20.04.1) 9.4.0, 64-bit` Is this normal?
