Re: Wrong statistics for size of XLOG_SWITCH during pg_waldump.
>I think that the length of the XLOG_SWITCH record is no other than 24 >bytes. Just adding the padding? garbage bytes to that length doesn't >seem the right thing to me. > >If we want pg_waldump to show that length somewhere, it could be shown >at the end of that record explicitly: > >rmgr: XLOGlen (rec/tot): 24/16776848, tx: 0, lsn: >0/02000148, prev 0/02000110, desc: SWITCH, trailing-bytes: 16776944 Thanks, I think it's good idea, and new patch attached. Here's the lookes: rmgr: XLOGlen (rec/tot): 24/24, tx: 0, lsn: 0/03D8, prev 0/0360, desc: SWITCH, trailing-bytes: 16776936 Regards, Highgo Software (Canada/China/Pakistan) URL : www.highgo.ca EMAIL: mailto:movead(dot)li(at)highgo(dot)ca fix_waldump_size_for_wal_switch_v2.patch Description: Binary data
Re: SQL-standard function body
On 2020-09-29 07:42, Michael Paquier wrote:
On Mon, Sep 07, 2020 at 08:00:08AM +0200, Peter Eisentraut wrote:
Some conflicts have emerged, so here is an updated patch.
I have implemented/fixed the inlining of set-returning functions written in
the new style, which was previously marked TODO in the patch.
The CF bot is telling that this patch fails to apply. Could you send
a rebase?
Here is a rebase, no functionality changes.
As indicated earlier, I'll also send some sub-patches as separate
submissions.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 14be6cb14759636b407f89cee50fa2896ee98f5d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sat, 10 Oct 2020 10:36:28 +0200
Subject: [PATCH v4] SQL-standard function body
This adds support for writing CREATE FUNCTION and CREATE PROCEDURE
statements for language SQL with a function body that conforms to the
SQL standard and is portable to other implementations.
Instead of the PostgreSQL-specific AS $$ string literal $$ syntax,
this allows writing out the SQL statements making up the body
unquoted, either as a single statement:
CREATE FUNCTION add(a integer, b integer) RETURNS integer
LANGUAGE SQL
RETURN a + b;
or as a block
CREATE PROCEDURE insert_data(a integer, b integer)
LANGUAGE SQL
BEGIN ATOMIC
INSERT INTO tbl VALUES (a);
INSERT INTO tbl VALUES (b);
END;
The function body is parsed at function definition time and stored as
expression nodes in probin. So at run time, no further parsing is
required.
However, this form does not support polymorphic arguments, because
there is no more parse analysis done at call time.
Dependencies between the function and the objects it uses are fully
tracked.
A new RETURN statement is introduced. This can only be used inside
function bodies. Internally, it is treated much like a SELECT
statement.
psql needs some new intelligence to keep track of function body
boundaries so that it doesn't send off statements when it sees
semicolons that are inside a function body.
Also, per SQL standard, LANGUAGE SQL is the default, so it does not
need to be specified anymore.
Discussion:
https://www.postgresql.org/message-id/flat/1c11f1eb-f00c-43b7-799d-2d44132c0...@2ndquadrant.com
---
doc/src/sgml/ref/create_function.sgml | 126 +--
doc/src/sgml/ref/create_procedure.sgml| 62 +-
src/backend/catalog/pg_proc.c | 145 +++--
src/backend/commands/aggregatecmds.c | 2 +
src/backend/commands/functioncmds.c | 98 +++--
src/backend/executor/functions.c | 79 ---
src/backend/nodes/copyfuncs.c | 15 ++
src/backend/nodes/equalfuncs.c| 13 ++
src/backend/nodes/outfuncs.c | 12 ++
src/backend/nodes/readfuncs.c | 1 +
src/backend/optimizer/util/clauses.c | 100 ++---
src/backend/parser/analyze.c | 35
src/backend/parser/gram.y | 128 +---
src/backend/tcop/postgres.c | 3 +-
src/backend/utils/adt/ruleutils.c | 110 +-
src/bin/pg_dump/pg_dump.c | 45 +++-
src/fe_utils/psqlscan.l | 23 ++-
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_proc.h | 2 +-
src/include/commands/defrem.h | 2 +
src/include/executor/functions.h | 15 ++
src/include/fe_utils/psqlscan_int.h | 2 +
src/include/nodes/nodes.h | 1 +
src/include/nodes/parsenodes.h| 13 ++
src/include/parser/kwlist.h | 2 +
src/include/tcop/tcopprot.h | 1 +
src/interfaces/ecpg/preproc/ecpg.addons | 6 +
src/interfaces/ecpg/preproc/ecpg.trailer | 4 +-
.../regress/expected/create_function_3.out| 195 +-
.../regress/expected/create_procedure.out | 58 ++
src/test/regress/sql/create_function_3.sql| 94 +
src/test/regress/sql/create_procedure.sql | 26 +++
32 files changed, 1209 insertions(+), 213 deletions(-)
diff --git a/doc/src/sgml/ref/create_function.sgml
b/doc/src/sgml/ref/create_function.sgml
index 3c1eaea651..1b5b9420db 100644
--- a/doc/src/sgml/ref/create_function.sgml
+++ b/doc/src/sgml/ref/create_function.sgml
@@ -38,6 +38,7 @@
| SET configuration_parameter
{ TO value | = value | FROM CURRENT }
| AS 'definition'
| AS 'obj_file',
'link_symbol'
+| sql_body
} ...
@@ -257,8 +258,9 @@ Parameters
The name of the language that the function is implemented in.
It can be sql, c,
internal, or the name of a user-defined
- procedural language, e.g., plpgsql. Enclosing the
- name in single quotes is deprecated and requires matching case.
+
Make LANGUAGE SQL the default
A sub-patch extracted from the bigger patch in thread "SQL-standard
function body"[0]: Make LANGUAGE SQL the default in CREATE FUNCTION and
CREATE PROCEDURE, per SQL standard.
[0]:
https://www.postgresql.org/message-id/flat/1c11f1eb-f00c-43b7-799d-2d44132c0...@2ndquadrant.com
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From f1cd36936ef18acda92258edecaeb4d0b83adea9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sat, 10 Oct 2020 10:45:02 +0200
Subject: [PATCH] Make LANGUAGE SQL the default
LANGUAGE SQL is the default in CREATE FUNCTION and CREATE PROCEDURE,
per SQL standard.
---
doc/src/sgml/ref/create_function.sgml | 5 +++--
doc/src/sgml/ref/create_procedure.sgml | 5 +++--
src/backend/commands/functioncmds.c | 11 ++-
src/test/regress/expected/create_function_3.out | 4 ++--
src/test/regress/sql/create_function_3.sql | 4 ++--
5 files changed, 12 insertions(+), 17 deletions(-)
diff --git a/doc/src/sgml/ref/create_function.sgml
b/doc/src/sgml/ref/create_function.sgml
index 3c1eaea651..79753e3454 100644
--- a/doc/src/sgml/ref/create_function.sgml
+++ b/doc/src/sgml/ref/create_function.sgml
@@ -257,8 +257,9 @@ Parameters
The name of the language that the function is implemented in.
It can be sql, c,
internal, or the name of a user-defined
- procedural language, e.g., plpgsql. Enclosing the
- name in single quotes is deprecated and requires matching case.
+ procedural language, e.g., plpgsql. The default is
+ sql. Enclosing the name in single quotes is
+ deprecated and requires matching case.
diff --git a/doc/src/sgml/ref/create_procedure.sgml
b/doc/src/sgml/ref/create_procedure.sgml
index e258eca5ce..a2b20e989c 100644
--- a/doc/src/sgml/ref/create_procedure.sgml
+++ b/doc/src/sgml/ref/create_procedure.sgml
@@ -162,8 +162,9 @@ Parameters
The name of the language that the procedure is implemented in.
It can be sql, c,
internal, or the name of a user-defined
- procedural language, e.g., plpgsql. Enclosing the
- name in single quotes is deprecated and requires matching case.
+ procedural language, e.g., plpgsql. The default is
+ sql. Enclosing the name in single quotes is
+ deprecated and requires matching case.
diff --git a/src/backend/commands/functioncmds.c
b/src/backend/commands/functioncmds.c
index c3ce480c8f..1fafb29377 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -797,17 +797,9 @@ compute_function_attributes(ParseState *pstate,
*as = NIL; /* keep compiler quiet
*/
}
+ /* process optional items */
if (language_item)
*language = strVal(language_item->arg);
- else
- {
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
-errmsg("no language specified")));
- *language = NULL; /* keep compiler quiet */
- }
-
- /* process optional items */
if (transform_item)
*transform = transform_item->arg;
if (windowfunc_item)
@@ -962,6 +954,7 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
get_namespace_name(namespaceId));
/* Set default attributes */
+ language = "sql";
isWindowFunc = false;
isStrict = false;
security = false;
diff --git a/src/test/regress/expected/create_function_3.out
b/src/test/regress/expected/create_function_3.out
index ce508ae1dc..e0a7715c56 100644
--- a/src/test/regress/expected/create_function_3.out
+++ b/src/test/regress/expected/create_function_3.out
@@ -14,11 +14,11 @@ SET search_path TO temp_func_test, public;
--
-- ARGUMENT and RETURN TYPES
--
-CREATE FUNCTION functest_A_1(text, date) RETURNS bool LANGUAGE 'sql'
+CREATE FUNCTION functest_A_1(text, date) RETURNS bool LANGUAGE SQL
AS 'SELECT $1 = ''abcd'' AND $2 > ''2001-01-01''';
CREATE FUNCTION functest_A_2(text[]) RETURNS int LANGUAGE 'sql'
AS 'SELECT $1[1]::int';
-CREATE FUNCTION functest_A_3() RETURNS bool LANGUAGE 'sql'
+CREATE FUNCTION functest_A_3() RETURNS bool
AS 'SELECT false';
SELECT proname, prorettype::regtype, proargtypes::regtype[] FROM pg_proc
WHERE oid in ('functest_A_1'::regproc,
diff --git a/src/test/regress/sql/create_function_3.sql
b/src/test/regress/sql/create_function_3.sql
index bd108a918f..7515ae080a 100644
--- a/src/test/regress/sql/create_function_3.sql
+++ b/src/test/regress/sql/create_function_3.sql
@@ -20,11 +20,11 @@ CREATE SCHEMA temp_func_test;
--
-- ARGUMENT and RETURN TYPES
--
-CREATE FUNCTION functest_A_1(text, date) RETURNS bool LANGUAGE 'sql'
+CREATE FUNCTION f
Re: BUG #15858: could not stat file - over 4GB
On Fri, Oct 9, 2020 at 10:22 PM Tom Lane wrote: > Emil Iggland writes: > > I tested the patch at hand, and it performs as expected. Files larger > than 4GB can be imported. > > Thanks for testing! > Thanks for testing! +1 > > I'd been expecting one of our Windows-savvy committers to pick this up, > but since nothing has been happening, I took it on myself to push it. > I'll probably regret that :-( > Thanks for taking care of this. I see no problems in the build farm, but please reach me if I missed something. > > I made a few cosmetic changes, mostly reorganizing comments in a way > that made more sense to me. > > I was working on a new version, which was pgindent-friendlier and clearer about reporting an error when 'errno' is not informed. Please find attached a patch with those changes. Regards, Juan José Santamaría Flecha 0001-win32stat-indent-and-errormapping-v1.patch Description: Binary data
Re: Parallel INSERT (INTO ... SELECT ...)
On Fri, Oct 9, 2020 at 2:39 PM Amit Kapila wrote:
>
> On Tue, Oct 6, 2020 at 3:08 PM Greg Nancarrow wrote:
> >
> > On Mon, Oct 5, 2020 at 10:36 PM Dilip Kumar wrote:
> >
> > Also, I have attached a separate patch (requested by Andres Freund)
> > that just allows the underlying SELECT part of "INSERT INTO ... SELECT
> > ..." to be parallel.
> >
>
> It might be a good idea to first just get this patch committed, if
> possible. So, I have reviewed the latest version of this patch:
>
Few initial comments on 0004-ParallelInsertSelect:
1.
@@ -2049,11 +2049,6 @@ heap_prepare_insert(Relation relation,
HeapTuple tup, TransactionId xid,
* inserts in general except for the cases where inserts generate a new
* CommandId (eg. inserts into a table having a foreign key column).
*/
- if (IsParallelWorker())
- ereport(ERROR,
- (errcode(ERRCODE_INVALID_TRANSACTION_STATE),
- errmsg("cannot insert tuples in a parallel worker")));
-
I have speculated above [1] to see if we can change this Assert
condition instead of just removing it? Have you considered that
suggestion?
2.
@@ -764,12 +778,13 @@ GetCurrentCommandId(bool used)
if (used)
{
/*
- * Forbid setting currentCommandIdUsed in a parallel worker, because
- * we have no provision for communicating this back to the leader. We
- * could relax this restriction when currentCommandIdUsed was already
- * true at the start of the parallel operation.
+ * If in a parallel worker, only allow setting currentCommandIdUsed
+ * if currentCommandIdUsed was already true at the start of the
+ * parallel operation (by way of SetCurrentCommandIdUsed()), otherwise
+ * forbid setting currentCommandIdUsed because we have no provision
+ * for communicating this back to the leader.
*/
- Assert(!IsParallelWorker());
+ Assert(!(IsParallelWorker() && !currentCommandIdUsed));
currentCommandIdUsed = true;
}
Once we allowed this, won't the next CommandCounterIncrement() in the
worker will increment the commandId which will lead to using different
commandIds in worker and leader? Is that prevented in some way, if so,
how? Can we document the same?
3.
@@ -173,7 +173,7 @@ standard_ExecutorStart(QueryDesc *queryDesc, int eflags)
* against performing unsafe operations in parallel mode, but this gives a
* more user-friendly error message.
*/
- if ((XactReadOnly || IsInParallelMode()) &&
+ if ((XactReadOnly || (IsInParallelMode() &&
queryDesc->plannedstmt->commandType != CMD_INSERT)) &&
!(eflags & EXEC_FLAG_EXPLAIN_ONLY))
ExecCheckXactReadOnly(queryDesc->plannedstmt);
I don't think above change is correct. We need to extend the below
check in ExecCheckXactReadOnly() because otherwise, it can allow
Insert operations even when XactReadOnly is set which we don't want.
ExecCheckXactReadOnly()
{
..
if (plannedstmt->commandType != CMD_SELECT || plannedstmt->hasModifyingCTE)
PreventCommandIfParallelMode(CreateCommandName((Node *) plannedstmt));
..
}
4.
@@ -173,18 +175,20 @@ ExecSerializePlan(Plan *plan, EState *estate)
* PlannedStmt to start the executor.
- pstmt->hasReturning = false;
- pstmt->hasModifyingCTE = false;
+ pstmt->hasReturning = estate->es_plannedstmt->hasReturning;
+ pstmt->hasModifyingCTE = estate->es_plannedstmt->hasModifyingCTE;
Why change hasModifyingCTE?
5.
+ if (isParallelInsertLeader)
+ {
+ /* For Parallel INSERT, if there are BEFORE STATEMENT triggers,
+ * these must be fired by the leader, not the parallel workers.
+ */
The multi-line comment should start from the second line. I see a
similar problem at other places in the patch as well.
6.
@@ -178,6 +214,25 @@ ExecGather(PlanState *pstate)
node->pei,
gather->initParam);
+ if (isParallelInsertLeader)
+ {
+ /* For Parallel INSERT, if there are BEFORE STATEMENT triggers,
+ * these must be fired by the leader, not the parallel workers.
+ */
+ if (nodeModifyTableState->fireBSTriggers)
+ {
+ fireBSTriggers(nodeModifyTableState);
+ nodeModifyTableState->fireBSTriggers = false;
+
+ /*
+ * Disable firing of AFTER STATEMENT triggers by local
+ * plan execution (ModifyTable processing). These will be
+ * fired at end of Gather processing.
+ */
+ nodeModifyTableState->fireASTriggers = false;
+ }
+ }
Can we encapsulate this in a separate function? It seems a bit odd to
directly do this ExecGather.
7.
@@ -418,14 +476,25 @@ ExecShutdownGatherWorkers(GatherState *node)
void
ExecShutdownGather(GatherState *node)
{
- ExecShutdownGatherWorkers(node);
+ if (node->pei == NULL)
+ return;
- /* Now destroy the parallel context. */
- if (node->pei != NULL)
So after this patch if "node->pei == NULL" then we won't shutdown
workers here? Why so?
8. You have made changes related to trigger execution for Gather node,
don't we need similar changes for GatherMerge node?
9.
@@ -383,7 +444,21 @@ cost_gather(GatherPath *path, PlannerInfo *root,
/* Parallel setup and communication cost. */
startup_cost += parallel_setup_cost;
- run_cost += parallel_tuple_cost * path->path.rows;
+
+ /*
+ * For Parallel INSERT, pro
Re: [PATCH] Add `truncate` option to subscription commands
> On Oct 10, 2020, at 12:14 AM, Amit Kapila wrote: > > On Sat, Oct 10, 2020 at 12:24 AM David Christensen > wrote: >> >> -hackers, >> >> Enclosed find a patch to add a “truncate” option to subscription commands. >> >> When adding new tables to a subscription (either via `CREATE SUBSCRIPTION` >> or `REFRESH PUBLICATION`), tables on the target which are being newly >> subscribed will be truncated before the data copy step. This saves explicit >> coordination of a manual `TRUNCATE` on the target tables and allows the >> results of the initial data sync to be the same as on the publisher at the >> time of sync. >> > > So IIUC, this will either truncate all the tables for a particular > subscription or none? Correct, when creating or altering the subscription all newly added tables would be left alone (current behavior) or truncated (new functionality from the patch). > Is it possible that the user wants some of > those tables to be truncated which made me think what exactly made you > propose this feature? Basically, is it from user complaint, or is it > some optimization that you think will be helpful to users? This comes from my own experience with setting up/modifying subscriptions with adding many multiple additional tables, some of which had data in the subscribing node. I would have found this feature very helpful. Thanks, David
Re: BUG #15858: could not stat file - over 4GB
On Sat, Oct 10, 2020 at 01:31:21PM +0200, Juan José Santamaría Flecha wrote: > Thanks for taking care of this. I see no problems in the build farm, but > please reach me if I missed something. Thanks for continuing your work on this patch. I see no related failures in the buildfarm. - _dosmaperr(GetLastError()); + DWORD err = GetLastError(); + + /* report when not ERROR_SUCCESS */ + if (err == ERROR_FILE_NOT_FOUND || err == ERROR_PATH_NOT_FOUND) + errno = ENOENT; + else + _dosmaperr(err); Why are you changing that? The original coding is fine, as _dosmaperr() already maps ERROR_FILE_NOT_FOUND and ERROR_PATH_NOT_FOUND to ENOENT. - _dosmaperr(GetLastError()); + DWORD err = GetLastError(); + CloseHandle(hFile); + _dosmaperr(err); These parts are indeed incorrect. CloseHandle() could overwrite errno. -- Michael signature.asc Description: PGP signature
Re: BUG #15858: could not stat file - over 4GB
On Sat, Oct 10, 2020 at 2:24 PM Michael Paquier wrote: > > - _dosmaperr(GetLastError()); > + DWORD err = GetLastError(); > + > + /* report when not ERROR_SUCCESS */ > + if (err == ERROR_FILE_NOT_FOUND || err == > ERROR_PATH_NOT_FOUND) > + errno = ENOENT; > + else > + _dosmaperr(err); > Why are you changing that? The original coding is fine, as > _dosmaperr() already maps ERROR_FILE_NOT_FOUND and > ERROR_PATH_NOT_FOUND to ENOENT. > If the file does not exist there is no need to call _dosmaperr() and log the error. > > - _dosmaperr(GetLastError()); > + DWORD err = GetLastError(); > + >CloseHandle(hFile); > + _dosmaperr(err); > These parts are indeed incorrect. CloseHandle() could overwrite > errno. > The meaningful error should come from the previous call, and an error from CloseHandle() could mask it. Not sure it makes a difference anyhow. Regards, Juan José Santamaría Flecha
Re: Batching page logging during B-tree build
> 23 сент. 2020 г., в 23:29, Andres Freund написал(а): > > Hi, > > On 2020-09-23 11:19:18 -0700, Peter Geoghegan wrote: >> On Fri, Sep 18, 2020 at 8:39 AM Andrey M. Borodin >> wrote: >>> Here is PoC with porting that same routine to B-tree. It allows to build >>> B-trees ~10% faster on my machine. > > I wonder what the effect of logging WAL records as huge as this (~256kb) > is on concurrent sessions. I think it's possible that logging 32 pages > at once would cause latency increases for concurrent OLTP-ish > writes. And that a smaller batch size would reduce that, while still > providing most of the speedup. Indeed, on my machine performance is indistinguishable between batches of 8, 16 and 32 pages, while still observable (~3...8%) with 8 against 4. Best regards, Andrey Borodin,
Re: Make LANGUAGE SQL the default
Peter Eisentraut writes: > A sub-patch extracted from the bigger patch in thread "SQL-standard > function body"[0]: Make LANGUAGE SQL the default in CREATE FUNCTION and > CREATE PROCEDURE, per SQL standard. I'm suspicious of doing this, mainly because DO does not have that default. I think sticking with no-default is less likely to cause confusion. Moreover, I don't really believe that having a default value here is going to add any noticeable ease-of-use for anyone. What's much more likely to happen is that we'll start getting novice questions about whatever weird syntax errors you get when trying to feed plpgsql code to the sql-language function parser. (I don't know what they are exactly, but I'll bet a very fine dinner that they're less understandable to a novice than "no language specified".) I don't see any reason why we can't figure out that an unquoted function body is SQL, while continuing to make no assumptions about a body written as a string. The argument that defaulting to SQL makes the latter case SQL-compliant seems pretty silly anyway. I also continue to suspect that we are going to need to treat quoted and unquoted SQL as two different languages, possibly with not even the same semantics. If that's how things shake out, claiming that the quoted-SQL version is the default because spec becomes even sillier. regards, tom lane
Re: Make LANGUAGE SQL the default
so 10. 10. 2020 v 18:14 odesílatel Tom Lane napsal: > Peter Eisentraut writes: > > A sub-patch extracted from the bigger patch in thread "SQL-standard > > function body"[0]: Make LANGUAGE SQL the default in CREATE FUNCTION and > > CREATE PROCEDURE, per SQL standard. > > I'm suspicious of doing this, mainly because DO does not have that > default. I think sticking with no-default is less likely to cause > confusion. Moreover, I don't really believe that having a default value > here is going to add any noticeable ease-of-use for anyone. What's much > more likely to happen is that we'll start getting novice questions about > whatever weird syntax errors you get when trying to feed plpgsql code to > the sql-language function parser. (I don't know what they are exactly, > but I'll bet a very fine dinner that they're less understandable to a > novice than "no language specified".) > > I don't see any reason why we can't figure out that an unquoted function > body is SQL, while continuing to make no assumptions about a body written > as a string. The argument that defaulting to SQL makes the latter case > SQL-compliant seems pretty silly anyway. > +1 Pavel > I also continue to suspect that we are going to need to treat quoted > and unquoted SQL as two different languages, possibly with not even > the same semantics. If that's how things shake out, claiming that the > quoted-SQL version is the default because spec becomes even sillier. > > regards, tom lane > > >
Re: BUG #15858: could not stat file - over 4GB
=?UTF-8?Q?Juan_Jos=C3=A9_Santamar=C3=ADa_Flecha?= writes: > If the file does not exist there is no need to call _dosmaperr() and log > the error. I concur with Michael that it's inappropriate to make an end run around _dosmaperr() here. If you think that the DEBUG5 logging inside that is inappropriate, you should propose removing it outright. Pushed the rest of this. (pgindent behaved differently around PFN_NTQUERYINFORMATIONFILE today than it did yesterday. No idea why.) > The meaningful error should come from the previous call, and an error from > CloseHandle() could mask it. Not sure it makes a difference anyhow. Would CloseHandle() really touch errno at all? But this way is certainly safer, so done. regards, tom lane
Re: BUG #15858: could not stat file - over 4GB
On Sat, Oct 10, 2020 at 7:43 PM Tom Lane wrote: > > I concur with Michael that it's inappropriate to make an end run around > _dosmaperr() here. If you think that the DEBUG5 logging inside that > is inappropriate, you should propose removing it outright. > > Pushed the rest of this. > Great, thanks again to everyone who has taken some time to look into this. Regards, Juan José Santamaría Flecha
Re: BUG #15858: could not stat file - over 4GB
On Sat, Oct 10, 2020 at 09:00:27PM +0200, Juan José Santamaría Flecha wrote:
> Great, thanks again to everyone who has taken some time to look into this.
We are visibly not completely out of the woods yet, jacana is
reporting a compilation error:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana&dt=2020-10-10%2018%3A00%3A28
Oct 10 14:04:40
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/port/win32stat.c:
In function 'fileinfo_to_stat':
Oct 10 14:04:40
c:/mingw/msys/1.0/home/pgrunner/bf/root/HEAD/pgsql.build/../pgsql/src/port/win32stat.c:151:13:
error: 'BY_HANDLE_FILE_INFORMATION {aka struct
_BY_HANDLE_FILE_INFORMATION}' has no member named 'nFileSizeLowi'; did
you mean 'nFileSizeLow'?
Oct 10 14:04:40 fiData.nFileSizeLowi);
Oct 10 14:04:40 ^
Oct 10 14:04:40 nFileSizeLow
I don't have the time to check MinGW and HEAD now, so that's just a
heads-up.
--
Michael
signature.asc
Description: PGP signature
Re: BUG #15858: could not stat file - over 4GB
Michael Paquier writes: > We are visibly not completely out of the woods yet, jacana is > reporting a compilation error: Nah, I fixed that hours ago (961e07b8c). jacana must not have run again yet. regards, tom lane
Re: Parallel INSERT (INTO ... SELECT ...)
On Sat, Oct 10, 2020 at 5:25 PM Amit Kapila wrote: > > 8. You have made changes related to trigger execution for Gather node, > don't we need similar changes for GatherMerge node? > .. > > 10. Don't we need a change similar to cost_gather in > cost_gather_merge? It seems you have made only partial changes for > GatherMerge node. > Please ignore these two comments as we can't push Insert to workers when GatherMerge is involved as a leader backend does the final phase (merge the results by workers). So, we can only push the Select part of the statement. -- With Regards, Amit Kapila.
Re: Parallel INSERT (INTO ... SELECT ...)
On Sun, Oct 11, 2020 at 12:55 AM Amit Kapila wrote:
> + /*
> + * For Parallel INSERT, provided no tuples are returned from workers
> + * to gather/leader node, don't add a cost-per-row, as each worker
> + * parallelly inserts the tuples that result from its chunk of plan
> + * execution. This change may make the parallel plan cheap among all
> + * other plans, and influence the planner to consider this parallel
> + * plan.
> + */
> + if (!(IsA(path->subpath, ModifyTablePath) &&
> + castNode(ModifyTablePath, path->subpath)->operation == CMD_INSERT &&
> + castNode(ModifyTablePath, path->subpath)->returningLists != NULL))
> + {
> + run_cost += parallel_tuple_cost * path->path.rows;
> + }
>
> Isn't the last condition in above check "castNode(ModifyTablePath,
> path->subpath)->returningLists != NULL" should be
> "castNode(ModifyTablePath, path->subpath)->returningLists == NULL"
> instead? Because otherwise when there is returning list it won't count
> the cost for passing tuples via Gather node. This might be reason of
> what Thomas has seen in his recent email [2].
Yeah, I think this is trying to fix the problem too late. Instead, we
should fix the incorrect row estimates so we don't have to fudge it
later like that. For example, this should be estimating rows=0:
postgres=# explain analyze insert into s select * from t t1 join t t2 using (i);
...
Insert on s (cost=30839.08..70744.45 rows=1000226 width=4) (actual
time=2940.560..2940.562 rows=0 loops=1)
I think that should be done with something like this:
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -3583,16 +3583,11 @@ create_modifytable_path(PlannerInfo *root,
RelOptInfo *rel,
if (lc == list_head(subpaths)) /* first node? */
pathnode->path.startup_cost = subpath->startup_cost;
pathnode->path.total_cost += subpath->total_cost;
- pathnode->path.rows += subpath->rows;
+ if (returningLists != NIL)
+ pathnode->path.rows += subpath->rows;
total_size += subpath->pathtarget->width * subpath->rows;
}
- /*
-* Set width to the average width of the subpath outputs. XXX this is
-* totally wrong: we should report zero if no RETURNING, else an average
-* of the RETURNING tlist widths. But it's what happened historically,
-* and improving it is a task for another day.
-*/
if (pathnode->path.rows > 0)
total_size /= pathnode->path.rows;
pathnode->path.pathtarget->width = rint(total_size);
Re: powerpc pg_atomic_compare_exchange_u32_impl: error: comparison of integer expressions of different signedness (Re: pgsql: For all ppc compilers, implement compare_exchange and) fetch_add
On Fri, Oct 09, 2020 at 03:01:17AM -0700, Noah Misch wrote:
> On Fri, Oct 09, 2020 at 11:28:25AM +0200, Christoph Berg wrote:
> > pg-cron on powerpc/ppc64/ppc64el is raising this warning inside the
> > ppc atomics:
> >
> > gcc -Wall -Wmissing-prototypes -Wpointer-arith
> > -Wdeclaration-after-statement -Werror=vla -Wendif-labels
> > -Wmissing-format-attribute -Wimplicit-fallthrough=3 -Wformat-security
> > -fno-strict-aliasing -fwrapv -fexcess-precision=standard
> > -Wno-format-truncation -Wno-stringop-truncation -g -g -O2
> > -fstack-protector-strong -Wformat -Werror=format-security -g -O2
> > -fdebug-prefix-map=/<>=. -fstack-protector-strong -Wformat
> > -Werror=format-security -fPIC -std=c99 -Wall -Wextra -Werror
> > -Wno-unknown-warning-option -Wno-unused-parameter -Wno-maybe-uninitialized
> > -Wno-implicit-fallthrough -Iinclude -I/usr/include/postgresql -I. -I./
> > -I/usr/include/postgresql/13/server -I/usr/include/postgresql/internal
> > -Wdate-time -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -I/usr/include/libxml2 -c
> > -o src/job_metadata.o src/job_metadata.c
> > In file included from /usr/include/postgresql/13/server/port/atomics.h:74,
> > from /usr/include/postgresql/13/server/utils/dsa.h:17,
> > from
> > /usr/include/postgresql/13/server/nodes/tidbitmap.h:26,
> > from /usr/include/postgresql/13/server/access/genam.h:19,
> > from src/job_metadata.c:21:
> > /usr/include/postgresql/13/server/port/atomics/arch-ppc.h: In function
> > ‘pg_atomic_compare_exchange_u32_impl’:
> > /usr/include/postgresql/13/server/port/atomics/arch-ppc.h:97:42: error:
> > comparison of integer expressions of different signedness: ‘uint32’ {aka
> > ‘unsigned int’} and ‘int’ [-Werror=sign-compare]
> >97 | *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
> > | ^~
> > src/job_metadata.c: At top level:
> > cc1: note: unrecognized command-line option ‘-Wno-unknown-warning-option’
> > may have been intended to silence earlier diagnostics
> > cc1: all warnings being treated as errors
> >
> > Looking at the pg_atomic_compare_exchange_u32_impl, this looks like a
> > genuine problem:
> >
> > static inline bool
> > pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32 *ptr,
> > uint32 *expected, uint32 newval)
> > ...
> > if (__builtin_constant_p(*expected) &&
> > *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
> >
> > If *expected is an unsigned integer, comparing it to PG_INT16_MIN
> > which is a negative number doesn't make sense.
> >
> > src/include/c.h:#define PG_INT16_MIN(-0x7FFF-1)
>
> Agreed. I'll probably fix it like this:
The first attachment fixes the matter you've reported. While confirming that,
I observed that gcc builds don't even use the 64-bit code in arch-ppc.h.
Oops. The second attachment fixes that. I plan not to back-patch either of
these.
Author: Noah Misch
Commit: Noah Misch
Choose ppc compare_exchange constant path for more operand values.
The implementation uses smaller code when the "expected" operand is a
small constant, but the implementation needlessly defined the set of
acceptable constants more narrowly than the ABI does. Core PostgreSQL
and PGXN don't use the constant path at all, so this is future-proofing.
Reviewed by FIXME. Reported by Christoph Berg.
Discussion: https://postgr.es/m/20201009092825.gd889...@msg.df7cb.de
diff --git a/src/include/port/atomics/arch-ppc.h
b/src/include/port/atomics/arch-ppc.h
index 68e6603..9b2e499 100644
--- a/src/include/port/atomics/arch-ppc.h
+++ b/src/include/port/atomics/arch-ppc.h
@@ -94,7 +94,8 @@ pg_atomic_compare_exchange_u32_impl(volatile pg_atomic_uint32
*ptr,
#ifdef HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P
if (__builtin_constant_p(*expected) &&
- *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
+ (int32) *expected <= PG_INT16_MAX &&
+ (int32) *expected >= PG_INT16_MIN)
__asm__ __volatile__(
" sync\n"
" lwarx %0,0,%5 \n"
@@ -183,7 +184,8 @@ pg_atomic_compare_exchange_u64_impl(volatile
pg_atomic_uint64 *ptr,
/* Like u32, but s/lwarx/ldarx/; s/stwcx/stdcx/; s/cmpw/cmpd/ */
#ifdef HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P
if (__builtin_constant_p(*expected) &&
- *expected <= PG_INT16_MAX && *expected >= PG_INT16_MIN)
+ (int64) *expected <= PG_INT16_MAX &&
+ (int64) *expected >= PG_INT16_MIN)
__asm__ __volatile__(
" sync\n"
" ldarx %0,0,%5 \n"
diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 02397f2..09bc42a 100644
--- a/src/test/r
