Re: BRIN minmax multi - incorrect distance for infinite timestamp/date
On Fri, Oct 27, 2023 at 10:32 PM Tomas Vondra wrote: > > FWIW I've cleaned up and pushed all the patches we came up with this > thread. And I've backpatched all of them to 14+. > Thanks a lot Tomas. -- Best Wishes, Ashutosh Bapat
Re: Open a streamed block for transactional messages during decoding
On Thu, Oct 26, 2023 at 2:01 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Thursday, October 26, 2023 12:42 PM Amit Kapila
> wrote:
> >
> > On Tue, Oct 24, 2023 at 5:27 PM Zhijie Hou (Fujitsu)
> >
> > wrote:
> > >
> > > While reviewing the test_decoding code, I noticed that when
> > > skip_empty_xacts option is specified, it doesn't open the streaming
> > block( e.g.
> > > pg_output_stream_start) before streaming the transactional MESSAGE
> > > even if it's the first change in a streaming block.
> > >
> > > It looks inconsistent with what we do when streaming DML changes(e.g.
> > > pg_decode_stream_change()).
> > >
> > > Here is a small patch to open the stream block in this case.
> > >
> >
> > The change looks good to me though I haven't tested it yet. BTW, can we
> > change the comment: "Output stream start if we haven't yet, but only for the
> > transactional case." to "Output stream start if we haven't yet for
> > transactional
> > messages"?
>
> Thanks for the review and I changed this as suggested.
>
--- a/contrib/test_decoding/expected/stream.out
+++ b/contrib/test_decoding/expected/stream.out
@@ -29,7 +29,10 @@ COMMIT;
SELECT data FROM pg_logical_slot_get_changes('regression_slot',
NULL,NULL, 'include-xids', '0', 'skip-empty-xacts', '1',
'stream-changes', '1');
data
--
+ opening a streamed block for transaction
streaming message: transactional: 1 prefix: test, sz: 50
+ closing a streamed block for transaction
+ aborting streamed (sub)transaction
I was analyzing the reason for the additional message: "aborting
streamed (sub)transaction" in the above test and it seems to be due to
the below check in the function pg_decode_stream_abort():
if (data->skip_empty_xacts && !xact_wrote_changes)
return;
Before the patch, we won't be setting the 'xact_wrote_changes' flag in
txndata which is fixed now. So, this looks okay to me. However, I have
another observation in this code which is that for aborts or
subtransactions, we are not checking the flag 'stream_wrote_changes',
so we may end up emitting the abort message even when no actual change
has been streamed. I haven't tried to generate a test to verify this
observation, so I could be wrong as well but it is worth analyzing
such cases.
--
With Regards,
Amit Kapila.
RE: A recent message added to pg_upgade
On Monday, October 30, 2023 10:29 AM Kyotaro Horiguchi
wrote:
>
> At Fri, 27 Oct 2023 14:57:10 +0530, Amit Kapila
> wrote in
> > On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera
> wrote:
> > >
> > > On 2023-Oct-27, Kyotaro Horiguchi wrote:
> > >
> > > > @@ -1433,8 +1433,8 @@
> InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
> > > > {
> > > > ereport(ERROR,
> > > >
> errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > > > - errmsg("replication slots must not
> be invalidated during the upgrade"),
> > > > -
> errhint("\"max_slot_wal_keep_size\" must be set to -1 during the upgrade"));
> > >
> > > Hmm, if I read this code right, this error is going to be thrown by
> > > the checkpointer while finishing a checkpoint. Fortunately, the
> > > checkpoint record has already been written, but I don't know what
> > > would happen if this is thrown while trying to write the shutdown
> > > checkpoint. Probably nothing terribly good.
> > >
> > > I don't think this is useful. If the setting is invalid during
> > > binary upgrade, let's prevent it from being set at all right from
> > > the start of the upgrade process.
> >
> > We are already forcing the required setting
> > "max_slot_wal_keep_size=-1" during the upgrade similar to some of the
> > other settings like "full_page_writes". However, the user can provide
> > an option for "max_slot_wal_keep_size" in which case it will be
> > overridden. Now, I think (a) we can ensure that our setting always
> > takes precedence in this case. The other idea is (b) to parse the
> > user-provided options and check if "max_slot_wal_keep_size" has a
> > value different than expected and raise an error accordingly. Or we
> > can simply (c) document the usage of max_slot_wal_keep_size in the
> > upgrade. I am not sure whether it is worth complicating the code for
> > this as the user shouldn't be using such an option during the upgrade.
> > So, I think doing (a) and (c) could be simpler.
> > >
> > > In InvalidatePossiblyObsoleteSlot() we could have just an Assert()
> > > or elog(PANIC).
> > >
> >
> > Yeah, we can change to either of those.
>
> This discussion seems like a bit off from my point. I suggested adding a check
> for that setting when IsBinaryUpgraded is true at the GUC level as shown in
> the
> attached patch. I believe Álvaro made a similar suggestion. While the error
> message is somewhat succinct, I think it is sufficient given the low
> possilibility
> of the scenario and the fact that it cannot occur inadvertently.
Thanks for the diff and I think the approach basically works.
One notable behavior of this approach it will reject the GUC setting even if
there
are no slots on old cluster or user set the value to a big enough value which
doesn't cause invalidation. The behavior doesn't look bad to me but just
mention it
for reference.
Best Regards,
Hou zj
Re: COPY TO (FREEZE)?
HI,
Zhang Mingli
www.hashdata.xyz
On Oct 30, 2023 at 10:58 +0800, Bruce Momjian , wrote:
> On Mon, Oct 30, 2023 at 05:07:48AM +0800, Mingli Zhang wrote:
> >
> > Bruce Momjian 于2023年10月30日周一03:35写道：
> >
> > On Sun, Oct 29, 2023 at 02:50:37PM +0800, Mingli Zhang wrote:
> > > I guess you want to write “cannot be used with COPY TO”
> >
> > You are 100% correct. Updated patch attached.
> >
> > --
> > Bruce Momjian https://momjian.us
> > EDB https://enterprisedb.com
> >
> > Only you can decide what is important to you.
> >
> >
> >
> > (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> >
> > + errmsg("COPY FREEZE mode cannot be used with COPY FROM")));
> >
> > +
> >
> >
> > COPY FROM-> COPY TO
>
> Agreed, patch attached.
>
> --
> Bruce Momjian https://momjian.us
> EDB https://enterprisedb.com
>
> Only you can decide what is important to you.
LGTM.
Re: A recent message added to pg_upgade
On Mon, Oct 30, 2023 at 7:58 AM Kyotaro Horiguchi
wrote:
>
> At Fri, 27 Oct 2023 14:57:10 +0530, Amit Kapila
> wrote in
> > On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera
> > wrote:
> > >
> > > On 2023-Oct-27, Kyotaro Horiguchi wrote:
> > >
> > > > @@ -1433,8 +1433,8 @@
> > > > InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
> > > > {
> > > > ereport(ERROR,
> > > >
> > > > errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > > > - errmsg("replication slots must
> > > > not be invalidated during the upgrade"),
> > > > -
> > > > errhint("\"max_slot_wal_keep_size\" must be set to -1 during the
> > > > upgrade"));
> > >
> > > Hmm, if I read this code right, this error is going to be thrown by the
> > > checkpointer while finishing a checkpoint. Fortunately, the checkpoint
> > > record has already been written, but I don't know what would happen if
> > > this is thrown while trying to write the shutdown checkpoint. Probably
> > > nothing terribly good.
> > >
> > > I don't think this is useful. If the setting is invalid during binary
> > > upgrade, let's prevent it from being set at all right from the start of
> > > the upgrade process.
> >
> > We are already forcing the required setting
> > "max_slot_wal_keep_size=-1" during the upgrade similar to some of the
> > other settings like "full_page_writes". However, the user can provide
> > an option for "max_slot_wal_keep_size" in which case it will be
> > overridden. Now, I think (a) we can ensure that our setting always
> > takes precedence in this case. The other idea is (b) to parse the
> > user-provided options and check if "max_slot_wal_keep_size" has a
> > value different than expected and raise an error accordingly. Or we
> > can simply (c) document the usage of max_slot_wal_keep_size in the
> > upgrade. I am not sure whether it is worth complicating the code for
> > this as the user shouldn't be using such an option during the upgrade.
> > So, I think doing (a) and (c) could be simpler.
> > >
> > > In InvalidatePossiblyObsoleteSlot() we could have
> > > just an Assert() or elog(PANIC).
> > >
> >
> > Yeah, we can change to either of those.
>
> This discussion seems like a bit off from my point. I suggested adding
> a check for that setting when IsBinaryUpgraded is true at the GUC
> level as shown in the attached patch. I believe Álvaro made a similar
> suggestion. While the error message is somewhat succinct, I think it
> is sufficient given the low possilibility of the scenario and the fact
> that it cannot occur inadvertently.
>
I think we can simply change that error message to assert if we want
to go with the check hook idea of yours. BTW, can we add
GUC_check_errdetail() with a better message as some of the other check
function uses? Also, I guess we can add some comments or at least
refer to the existing comments to explain the reason of such a check.
--
With Regards,
Amit Kapila.
Re: COPY TO (FREEZE)?
On Mon, Oct 30, 2023 at 05:07:48AM +0800, Mingli Zhang wrote:
>
> Bruce Momjian 于2023年10月30日周一03:35写道：
>
> On Sun, Oct 29, 2023 at 02:50:37PM +0800, Mingli Zhang wrote:
> > I guess you want to write “cannot be used with COPY TO”
>
> You are 100% correct. Updated patch attached.
>
> --
> Bruce Momjian https://momjian.us
> EDB https://enterprisedb.com
>
> Only you can decide what is important to you.
>
>
>
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>
> + errmsg("COPY FREEZE mode cannot be used with COPY FROM")));
>
> +
>
>
> COPY FROM-> COPY TO
Agreed, patch attached.
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d12ba96497..82b65543c3 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -224,6 +224,7 @@ COPY { table_name [ ( COPY FREEZE on
a partitioned table.
+ This option is allowed only in COPY FROM.
Note that all other sessions will immediately be able to see the data
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c5d7d78645..8da4e6c226 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -711,8 +711,8 @@ ProcessCopyOptions(ParseState *pstate,
errmsg("COPY force not null available only in CSV mode")));
if (opts_out->force_notnull != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force not null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY force not null cannot be used with COPY TO")));
/* Check force_null */
if (!opts_out->csv_mode && opts_out->force_null != NIL)
@@ -722,22 +722,28 @@ ProcessCopyOptions(ParseState *pstate,
if (opts_out->force_null != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY force null cannot be used with COPY TO")));
/* Don't allow the delimiter to appear in the null string. */
if (strchr(opts_out->null_print, opts_out->delim[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("COPY delimiter must not appear in the NULL specification")));
/* Don't allow the CSV quote char to appear in the null string. */
if (opts_out->csv_mode &&
strchr(opts_out->null_print, opts_out->quote[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("CSV quote character must not appear in the NULL specification")));
+ /* Check freeze */
+ if (opts_out->freeze && !is_from)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FREEZE mode cannot be used with COPY TO")));
+
if (opts_out->default_print)
{
if (!is_from)
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 95ec7363af..98ef2ef140 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -89,11 +89,11 @@ ERROR: COPY force quote only available using COPY TO
COPY x to stdout (format TEXT, force_not_null(a));
ERROR: COPY force not null available only in CSV mode
COPY x to stdin (format CSV, force_not_null(a));
-ERROR: COPY force not null only available using COPY FROM
+ERROR: COPY force not null cannot be used with COPY TO
COPY x to stdout (format TEXT, force_null(a));
ERROR: COPY force null available only in CSV mode
COPY x to stdin (format CSV, force_null(a));
-ERROR: COPY force null only available using COPY FROM
+ERROR: COPY force null cannot be used with COPY TO
-- too many columns in column list: should fail
COPY x (a, b, c, d, e, d, c) from stdin;
ERROR: column "d" specified more than once
Re: A recent message added to pg_upgade
At Fri, 27 Oct 2023 14:57:10 +0530, Amit Kapila wrote
in
> On Fri, Oct 27, 2023 at 2:02 PM Alvaro Herrera
> wrote:
> >
> > On 2023-Oct-27, Kyotaro Horiguchi wrote:
> >
> > > @@ -1433,8 +1433,8 @@
> > > InvalidatePossiblyObsoleteSlot(ReplicationSlotInvalidationCause cause,
> > > {
> > > ereport(ERROR,
> > >
> > > errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > > - errmsg("replication slots must not
> > > be invalidated during the upgrade"),
> > > - errhint("\"max_slot_wal_keep_size\"
> > > must be set to -1 during the upgrade"));
> >
> > Hmm, if I read this code right, this error is going to be thrown by the
> > checkpointer while finishing a checkpoint. Fortunately, the checkpoint
> > record has already been written, but I don't know what would happen if
> > this is thrown while trying to write the shutdown checkpoint. Probably
> > nothing terribly good.
> >
> > I don't think this is useful. If the setting is invalid during binary
> > upgrade, let's prevent it from being set at all right from the start of
> > the upgrade process.
>
> We are already forcing the required setting
> "max_slot_wal_keep_size=-1" during the upgrade similar to some of the
> other settings like "full_page_writes". However, the user can provide
> an option for "max_slot_wal_keep_size" in which case it will be
> overridden. Now, I think (a) we can ensure that our setting always
> takes precedence in this case. The other idea is (b) to parse the
> user-provided options and check if "max_slot_wal_keep_size" has a
> value different than expected and raise an error accordingly. Or we
> can simply (c) document the usage of max_slot_wal_keep_size in the
> upgrade. I am not sure whether it is worth complicating the code for
> this as the user shouldn't be using such an option during the upgrade.
> So, I think doing (a) and (c) could be simpler.
> >
> > In InvalidatePossiblyObsoleteSlot() we could have
> > just an Assert() or elog(PANIC).
> >
>
> Yeah, we can change to either of those.
This discussion seems like a bit off from my point. I suggested adding
a check for that setting when IsBinaryUpgraded is true at the GUC
level as shown in the attached patch. I believe Álvaro made a similar
suggestion. While the error message is somewhat succinct, I think it
is sufficient given the low possilibility of the scenario and the fact
that it cannot occur inadvertently.
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 99823df3c7..b1495c4754 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -52,6 +52,7 @@
#include "storage/proc.h"
#include "storage/procarray.h"
#include "utils/builtins.h"
+#include "utils/guc_hooks.h"
/*
* Replication slot on-disk data structure.
@@ -111,6 +112,17 @@ static void RestoreSlotFromDisk(const char *name);
static void CreateSlotOnDisk(ReplicationSlot *slot);
static void SaveSlotToPath(ReplicationSlot *slot, const char *dir, int elevel);
+/*
+ * GUC check_hook for max_slot_wal_keep_size
+ */
+bool
+check_max_slot_wal_keep_size(int *newval, void **extra, GucSource source)
+{
+ if (IsBinaryUpgrade && *newval != -1)
+ return false;
+ return true;
+}
+
/*
* Report shared-memory space needed by ReplicationSlotsShmemInit.
*/
diff --git a/src/backend/utils/misc/guc_tables.c
b/src/backend/utils/misc/guc_tables.c
index 7605eff9b9..818ffdb2ae 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -2845,7 +2845,7 @@ struct config_int ConfigureNamesInt[] =
},
_slot_wal_keep_size_mb,
-1, -1, MAX_KILOBYTES,
- NULL, NULL, NULL
+ check_max_slot_wal_keep_size, NULL, NULL
},
{
diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h
index 2a191830a8..3d74483f44 100644
--- a/src/include/utils/guc_hooks.h
+++ b/src/include/utils/guc_hooks.h
@@ -84,6 +84,8 @@ extern bool check_maintenance_io_concurrency(int *newval,
void **extra,
extern void assign_maintenance_io_concurrency(int newval, void *extra);
extern bool check_max_connections(int *newval, void **extra, GucSource source);
extern bool check_max_wal_senders(int *newval, void **extra, GucSource source);
+extern bool check_max_slot_wal_keep_size(int *newval, void **extra,
+
GucSource source);
extern void assign_max_wal_size(int newval, void *extra);
extern bool check_max_worker_processes(int *newval, void **extra,
GucSource source);
Re: pgBufferUsage.blk_{read|write}_time are zero although there are pgBufferUsage.local_blks_{read|written}
On Fri, Oct 27, 2023 at 04:58:20PM +0300, Nazir Bilal Yavuz wrote: > I think switching it to 'shared' makes sense. That shouldn't confuse > existing monitoring queries much as the numbers won't change, right? > Also, if we keep 'shared/local' there could be similar complaints to > this thread in the future; so, at least adding comments can be > helpful. The problem is that it may impact existing tools that do explain output deparsing. One of them is https://explain.depesz.com/ that Hubert Depesz Lubaczewski has implemented, and it would be sad to break anything related to it. I am adding Hubert in CC for comments about changing this "shared/local" to "shared" on a released branch. Knowing that "shared" and "local" will need to be handled as separate terms in 17~ anyway, perhaps that's not a big deal, but let's be sure. -- Michael signature.asc Description: PGP signature
Re: Why is DEFAULT_FDW_TUPLE_COST so insanely low?
On Sun, 29 Oct 2023 at 12:45, Bruce Momjian wrote: > Has anything been done about this issue? Nothing has been done. I was hoping to get the attention of a few people who have dealt more with postgres_fdw in the past. I've attached a patch with adjusts DEFAULT_FDW_TUPLE_COST and sets it to 0.2. I set it to this because my experiment in [1] showed that it was about 21x lower than the actual costs (on my machine with a loopback fdw connecting to the same instance and database using my example query). Given that we have parallel_tuple_cost set to 0.1 by default, the network cost of a tuple from an FDW of 0.2 seems reasonable to me. Slightly higher is probably also reasonable, but given the seeming lack of complaints, I think I'd rather err on the low side. Changing it to 0.2, I see 4 plans change in postgres_fdw's regression tests. All of these changes are due to STD_FUZZ_FACTOR causing some other plan to win in add_path(). For example the query EXPLAIN (VERBOSE, ANALYZE) SELECT a, sum(b), min(b), count(*) FROM pagg_tab GROUP BY a HAVING avg(b) < 22 ORDER BY 1; the plan switches from a HashAggregate to a GroupAggregate. This is because after increasing the DEFAULT_FDW_TUPLE_COST to 0.2 the sorted append child (fuzzily) costs the same as the unsorted seq scan path and the sorted path wins in add_path due to having better pathkeys. The seq scan path is then thrown away and we end up doing the Group Aggregate using the sorted append children. If I change STD_FUZZ_FACTOR to something like 1.001 then the plans no longer change when I do: alter server loopback options (add fdw_tuple_cost '0.01'); alter server loopback options (drop fdw_tuple_cost); Ordinarily, I'd not care too much about that, but I did test the performance of one of the plans and the new plan came out slower than the old one. I'm not exactly sure how best to proceed on this in the absence of any feedback. David [1] https://postgr.es/m/caaphdvopvjjfh5c1ed2hrvddfom2depmwwiu5-f1anmyprj...@mail.gmail.com diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 144c114d0f..e689394807 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -9729,21 +9729,19 @@ SELECT t1.a, t1.phv, t2.b, t2.phv FROM (SELECT 't1_phv' phv, * FROM fprt1 WHERE -- test FOR UPDATE; partitionwise join does not apply EXPLAIN (COSTS OFF) SELECT t1.a, t2.b FROM fprt1 t1 INNER JOIN fprt2 t2 ON (t1.a = t2.b) WHERE t1.a % 25 = 0 ORDER BY 1,2 FOR UPDATE OF t1; - QUERY PLAN --- + QUERY PLAN + LockRows - -> Sort - Sort Key: t1.a - -> Hash Join - Hash Cond: (t2.b = t1.a) + -> Nested Loop + Join Filter: (t1.a = t2.b) + -> Append + -> Foreign Scan on ftprt1_p1 t1_1 + -> Foreign Scan on ftprt1_p2 t1_2 + -> Materialize -> Append -> Foreign Scan on ftprt2_p1 t2_1 -> Foreign Scan on ftprt2_p2 t2_2 - -> Hash - -> Append - -> Foreign Scan on ftprt1_p1 t1_1 - -> Foreign Scan on ftprt1_p2 t1_2 -(12 rows) +(10 rows) SELECT t1.a, t2.b FROM fprt1 t1 INNER JOIN fprt2 t2 ON (t1.a = t2.b) WHERE t1.a % 25 = 0 ORDER BY 1,2 FOR UPDATE OF t1; a | b @@ -9778,18 +9776,16 @@ ANALYZE fpagg_tab_p3; SET enable_partitionwise_aggregate TO false; EXPLAIN (COSTS OFF) SELECT a, sum(b), min(b), count(*) FROM pagg_tab GROUP BY a HAVING avg(b) < 22 ORDER BY 1; -QUERY PLAN - Sort - Sort Key: pagg_tab.a - -> HashAggregate - Group Key: pagg_tab.a - Filter: (avg(pagg_tab.b) < '22'::numeric) - -> Append - -> Foreign Scan on fpagg_tab_p1 pagg_tab_1 - -> Foreign Scan on fpagg_tab_p2 pagg_tab_2 - -> Foreign Scan on fpagg_tab_p3 pagg_tab_3 -(9 rows) + QUERY PLAN +- + GroupAggregate + Group Key: pagg_tab.a + Filter: (avg(pagg_tab.b) < '22'::numeric) + -> Append + -> Foreign Scan on fpagg_tab_p1 pagg_tab_1 + -> Foreign Scan on fpagg_tab_p2 pagg_tab_2 + -> Foreign Scan on fpagg_tab_p3 pagg_tab_3 +(7 rows) -- Plan with partitionwise aggregates is enabled SET enable_partitionwise_aggregate TO true; @@ -9823,34 +9819,32 @@ SELECT a, sum(b), min(b), count(*) FROM pagg_tab GROUP BY a HAVING avg(b) < 22 O -- Should have all the columns in the target list for the given relation EXPLAIN (VERBOSE, COSTS OFF) SELECT a,
Re: Introduce a new view for checkpointer related stats
On Fri, Oct 27, 2023 at 10:23:34AM +0530, Bharath Rupireddy wrote:
> +1. Changed in the attached v10-001. FWIW, having a test case in
> stats.sql emitting this error message and hint would have helped here.
> If okay, I can add one.
That may be something to do. At least it was missed on this thread,
even if we don't add a new pgstat shared type every day.
> PS: I'll park the SLRU flush related patch aside until the approach is
> finalized. I'm posting the pg_stat_checkpointer patch as v10-0001.
+-- Test that reset_shared with checkpointer specified as the stats type works
+SELECT stats_reset AS checkpointer_reset_ts FROM pg_stat_checkpointer \gset
+SELECT pg_stat_reset_shared('checkpointer');
+SELECT stats_reset > :'checkpointer_reset_ts'::timestamptz FROM
pg_stat_checkpointer;
+SELECT stats_reset AS checkpointer_reset_ts FROM pg_stat_checkpointer \gset
Note that you have forgotten to update the test of
pg_stat_reset_shared(NULL) to check for the value of
checkpointer_reset_ts. I've added an extra SELECT to check that for
pg_stat_checkpointer, and applied v8.
--
Michael
signature.asc
Description: PGP signature
Re: SQL:2011 application time
hi.
* The attached patch makes foreign keys with PERIOD fail if any of the
foreign key columns is "generated columns".
* The following queries will cause segmentation fault. not sure the
best way to fix it. the reason
in LINE: numpks = transformColumnNameList(RelationGetRelid(pkrel),
fkconstraint->pk_attrs, pkattnum, pktypoid);
begin;
drop table if exists temporal3,temporal_fk_rng2rng;
CREATE TABLE temporal3 (id int4range,valid_at tsrange,
CONSTRAINT temporal3_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS));
CREATE TABLE temporal_fk_rng2rng (
id int4range,valid_at tsrange,parent_id int4range,
CONSTRAINT temporal_fk_rng2rng_pk PRIMARY KEY (id, valid_at WITHOUT OVERLAPS),
CONSTRAINT temporal_fk_rng2rng_fk FOREIGN KEY (parent_id, PERIOD valid_at)
REFERENCES temporal3 (id, valid_at)
);
* change the function FindFKComparisonOperators's "eqstrategy" to
make pg_constraint record correct {conpfeqop,conppeqop,conffeqop}.
* fix the ON DELETE SET NULL/DEFAULT (columnlist). Now the following
queries error will be more consistent.
ALTER TABLE temporal_fk_rng2rng DROP CONSTRAINT temporal_fk_rng2rng_fk,
ADD CONSTRAINT temporal_fk_rng2rng_fk
FOREIGN KEY (parent_id, PERIOD valid_at) REFERENCES temporal_rng
ON DELETE SET DEFAULT(valid_at);
--ON DELETE SET NULL(valid_at);
* refactor restrict_cascading_range function.
* you did if (numfks != numpks) before if (is_temporal) {numfks +=
1;}, So I changed the code order to make the error report more
consistent.
anyway, I put it in one patch. please check the attached.
From 7dc2194f9bd46cfee7cfc774a2e52a2b64d465ea Mon Sep 17 00:00:00 2001
From: pgaddict
Date: Sun, 29 Oct 2023 14:42:57 +0800
Subject: [PATCH v1 1/1] various small fixes.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* The attached patch makes foreign keys with PERIOD fail if any of the foreign key columns is "generated columns".
* change the function FindFKComparisonOperators's "eqstrategy" to make pg_constraint record correct {conpfeqop,conppeqop,conffeqop}.
* refactor restrict_cascading_range function.
* first (is_temporal) {numfks += 1;} then (numfks != numpks) validation.
* variable comment fix.
---
src/backend/commands/tablecmds.c| 96 -
src/backend/utils/adt/ri_triggers.c | 32 +-
2 files changed, 69 insertions(+), 59 deletions(-)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 5ae0f113..86e99c81 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -522,7 +522,7 @@ static ObjectAddress addFkRecurseReferenced(List **wqueue, Constraint *fkconstra
bool is_temporal);
static void validateFkOnDeleteSetColumns(int numfks, const int16 *fkattnums,
int numfksetcols, const int16 *fksetcolsattnums,
- List *fksetcols);
+ const int16 *fkperiodattnums, List *fksetcols);
static void addFkRecurseReferencing(List **wqueue, Constraint *fkconstraint,
Relation rel, Relation pkrel, Oid indexOid, Oid parentConstr,
int numfks, int16 *pkattnum, int16 *fkattnum,
@@ -10292,6 +10292,7 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
fkdelsetcols, NULL);
validateFkOnDeleteSetColumns(numfks, fkattnum,
numfkdelsetcols, fkdelsetcols,
+ fkperiodattnums,
fkconstraint->fk_del_set_cols);
/*
@@ -10325,6 +10326,43 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
opclasses);
}
+ /*
+ * On the strength of a previous constraint, we might avoid scanning
+ * tables to validate this one. See below.
+ */
+ old_check_ok = (fkconstraint->old_conpfeqop != NIL);
+ Assert(!old_check_ok || numfks == list_length(fkconstraint->old_conpfeqop));
+
+ for (i = 0; i < numpks; i++)
+ {
+ FindFKComparisonOperators(
+fkconstraint, tab, i, fkattnum,
+_check_ok, _pfeqop_item,
+pktypoid[i], fktypoid[i], opclasses[i],
+is_temporal, false,
+[i], [i], [i]);
+ }
+ /* set pfeq, ppeq, ffeq operators for withoutoverlaps constraint.
+ * this also assume overlaps is the last key columns in constraint.
+ *
+ */
+ if (is_temporal)
+ {
+ pkattnum[numpks] = pkperiodattnums[0];
+ pktypoid[numpks] = pkperiodtypoids[0];
+ fkattnum[numpks] = fkperiodattnums[0];
+ fktypoid[numpks] = fkperiodtypoids[0];
+
+ FindFKComparisonOperators(
+fkconstraint, tab, numpks, fkattnum,
+_check_ok, _pfeqop_item,
+pkperiodtypoids[0], fkperiodtypoids[0], opclasses[numpks],
+is_temporal, true,
+[numpks], [numpks], [numpks]);
+ numfks += 1;
+ numpks += 1;
+ }
+
/*
* Now we can check permissions.
*/
@@ -10371,38 +10409,6 @@ ATAddForeignKeyConstraint(List **wqueue, AlteredTableInfo *tab, Relation rel,
(errcode(ERRCODE_INVALID_FOREIGN_KEY),
errmsg("number of referencing and referenced columns for foreign key disagree")));
- /*
- * On the strength of a previous constraint, we
Re: Cleaning up array_in()
rebase after commit
(https://git.postgresql.org/cgit/postgresql.git/commit/?id=611806cd726fc92989ac918eac48fd8d684869c7)
From d37081ba00743585dae35c70e293ce2385201c9c Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas
Date: Sat, 8 Jul 2023 22:19:48 +0300
Subject: [PATCH v9 5/7] Determine array dimensions and parse the elements in
one pass.
The logic in ArrayCount() and ReadArrayStr() was hard to follow,
because they mixed low-level escaping and quoting with tracking the
curly braces. Introduce a ReadArrayToken() function that tokenizes the
input, handling quoting and escaping.
Instead of having separate ArrayCount() and ReadArrayStr() functions, move
the logic for determining and checking the structure of the dimensions
to ReadArrayStr(). ReadArrayStr() now determines the dimensions and
parses the elements in one pass.
ReadArrayStr() no longer scribbles on the input. Instead, it allocates
a separate scratch buffer that it uses to hold the string representation
of each element. The scratch buffer must be as large as the input string,
so this doesn't save memory, but palloc(strlen(x)) is cheaper than
pstrdup(x).
---
src/backend/utils/adt/arrayfuncs.c | 1044 +++---
src/test/regress/expected/arrays.out |2 +-
src/tools/pgindent/typedefs.list |2 +-
3 files changed, 440 insertions(+), 608 deletions(-)
diff --git a/src/backend/utils/adt/arrayfuncs.c b/src/backend/utils/adt/arrayfuncs.c
index 7a63db09..a2d806e3 100644
--- a/src/backend/utils/adt/arrayfuncs.c
+++ b/src/backend/utils/adt/arrayfuncs.c
@@ -53,18 +53,6 @@ bool Array_nulls = true;
PG_FREE_IF_COPY(array, n); \
} while (0)
-typedef enum
-{
- ARRAY_NO_LEVEL,
- ARRAY_LEVEL_STARTED,
- ARRAY_ELEM_STARTED,
- ARRAY_QUOTED_ELEM_STARTED,
- ARRAY_QUOTED_ELEM_COMPLETED,
- ARRAY_ELEM_DELIMITED,
- ARRAY_LEVEL_COMPLETED,
- ARRAY_LEVEL_DELIMITED,
-} ArrayParseState;
-
/* Working state for array_iterate() */
typedef struct ArrayIteratorData
{
@@ -89,18 +77,30 @@ typedef struct ArrayIteratorData
int current_item; /* the item # we're at in the array */
} ArrayIteratorData;
+/* ReadArrayToken return type */
+typedef enum
+{
+ ATOK_LEVEL_START,
+ ATOK_LEVEL_END,
+ ATOK_DELIM,
+ ATOK_ELEM,
+ ATOK_ELEM_NULL,
+ ATOK_ERROR,
+} ArrayToken;
+
static bool ReadDimensionInt(char **srcptr, int *result, const char *origStr, Node *escontext);
static bool ReadArrayDimensions(char **srcptr, int *ndim, int *dim, int *lBound,
const char *origStr, Node *escontext);
-static int ArrayCount(const char *str, int *dim, char typdelim,
- Node *escontext);
-static bool ReadArrayStr(char *arrayStr, const char *origStr,
- int nitems,
+static bool ReadArrayStr(char **srcptr,
FmgrInfo *inputproc, Oid typioparam, int32 typmod,
char typdelim,
int typlen, bool typbyval, char typalign,
- Datum *values, bool *nulls,
- bool *hasnulls, int32 *nbytes, Node *escontext);
+ int *ndim, int *dim,
+ int *nitems,
+ Datum **values, bool **nulls,
+ const char *origStr, Node *escontext);
+static ArrayToken ReadArrayToken(char **srcptr, char *elembuf, char typdelim,
+ const char *origStr, Node *escontext);
static void ReadArrayBinary(StringInfo buf, int nitems,
FmgrInfo *receiveproc, Oid typioparam, int32 typmod,
int typlen, bool typbyval, char typalign,
@@ -186,12 +186,11 @@ array_in(PG_FUNCTION_ARGS)
char typalign;
char typdelim;
Oid typioparam;
- char *string_save,
- *p;
- int i,
-nitems;
- Datum *dataPtr;
- bool *nullsPtr;
+ char *p;
+ int nitems;
+ int nitems_according_to_dims;
+ Datum *values;
+ bool *nulls;
bool hasnulls;
int32 nbytes;
int32 dataoffset;
@@ -234,15 +233,13 @@ array_in(PG_FUNCTION_ARGS)
typdelim = my_extra->typdelim;
typioparam = my_extra->typioparam;
- /* Make a modifiable copy of the input */
- string_save = pstrdup(string);
-
/*
+ * Start processing the input string.
+ *
* If the input string starts with dimension info, read and use that.
- * Otherwise, we require the input to be in curly-brace style, and we
- * prescan the input to determine dimensions.
+ * Otherwise, we determine the dimensions from the curly braces.
*/
- p = string_save;
+ p = string;
if (!ReadArrayDimensions(, , dim, lBound, string, escontext))
return (Datum) 0;
@@ -254,17 +251,11 @@ array_in(PG_FUNCTION_ARGS)
(errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
errmsg("malformed array literal: \"%s\"", string),
errdetail("Array value must start with \"{\" or dimension information.")));
- ndim = ArrayCount(p, dim, typdelim, escontext);
- if (ndim < 0)
- PG_RETURN_NULL();
- for (i = 0; i < ndim; i++)
+ for (int i = 0; i < MAXDIM; i++)
lBound[i] = 1;
}
else
{
- int ndim_braces,
- dim_braces[MAXDIM];
-
/* If array dimensions are given, expect '=' operator */
if (strncmp(p, ASSGN, strlen(ASSGN)) != 0)
ereturn(escontext, (Datum) 0,
@@
Re: Reconcile stats in find_tabstat_entry() and get rid of PgStat_BackendFunctionEntry
On Fri, Oct 27, 2023 at 09:45:52AM +0200, Drouvot, Bertrand wrote: > Done in V8 attached (pgindent has been run on pgstatfuncs.c and > pgstat_relation.c). And applied that after editing a bit the comments. -- Michael signature.asc Description: PGP signature
Re: On login trigger: take three
Alexander Korotkov writes: > On Sun, Oct 29, 2023 at 6:16 PM Tom Lane wrote: >> It looks to me that what happened here is that the backend completed the >> authentication handshake, and then the login trigger caused a FATAL exit, >> and after than the connected psql session tried to send "SELECT 1" on >> an already-closed pipe. That failed, causing IPC::Run to panic. Looking closer, what must have happened is that the psql session ended before IPC::Run could jam 'SELECT 1' into its stdin. I wonder if this could be stabilized by doing 'psql -c "SELECT 1"' and not having to write anything to the child process stdin? But I'm not sure this test case is valuable enough to put a great deal of work into. >> mamba is a fairly slow machine and doubtless has timing a bit different >> from what this test was created on. But I doubt there is any way to make >> this behavior perfectly stable across a range of machines, so I recommend >> just removing the test case involving a fatal exit. > Makes sense. Are you good with the attached patch? OK by me, though I note you misspelled "mallory" in the comment. regards, tom lane
Re: On login trigger: take three
On Sun, Oct 29, 2023 at 6:16 PM Tom Lane wrote: > Mikhail Gribkov writes: > > Just for a more complete picture of the final state here. > > I have posted the described fix (for avoiding race condition in the tests) > > separately: > > https://commitfest.postgresql.org/45/4616/ > > It turns out that the TAP test for this feature (006_login_trigger.pl) > also has a race condition: > > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba=2023-10-28%2003%3A33%3A28 > > The critical bit of the log is > > ack Broken pipe: write( 14, 'SELECT 1;' ) at > /usr/pkg/lib/perl5/vendor_perl/5.36.0/IPC/Run/IO.pm line 550. > > It looks to me that what happened here is that the backend completed the > authentication handshake, and then the login trigger caused a FATAL exit, > and after than the connected psql session tried to send "SELECT 1" on > an already-closed pipe. That failed, causing IPC::Run to panic. > > mamba is a fairly slow machine and doubtless has timing a bit different > from what this test was created on. But I doubt there is any way to make > this behavior perfectly stable across a range of machines, so I recommend > just removing the test case involving a fatal exit. Makes sense. Are you good with the attached patch? -- Regards, Alexander Korotkov fix_login_trigger_test_instability.patch Description: Binary data
Re: COPY TO (FREEZE)?
Bruce Momjian 于2023年10月30日 周一03:35写道：
> On Sun, Oct 29, 2023 at 02:50:37PM +0800, Mingli Zhang wrote:
> > I guess you want to write “cannot be used with COPY TO”
>
> You are 100% correct. Updated patch attached.
>
> --
> Bruce Momjian https://momjian.us
> EDB https://enterprisedb.com
>
> Only you can decide what is important to you.
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>
> + errmsg("COPY FREEZE mode cannot be used with COPY FROM")));
>
> +
>
>
COPY FROM-> COPY TO
>
Re: COPY TO (FREEZE)?
On Sun, Oct 29, 2023 at 02:50:37PM +0800, Mingli Zhang wrote:
> I guess you want to write “cannot be used with COPY TO”
You are 100% correct. Updated patch attached.
--
Bruce Momjian https://momjian.us
EDB https://enterprisedb.com
Only you can decide what is important to you.
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index d12ba96497..82b65543c3 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -224,6 +224,7 @@ COPY { table_name [ ( COPY FREEZE on
a partitioned table.
+ This option is allowed only in COPY FROM.
Note that all other sessions will immediately be able to see the data
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index c5d7d78645..bb6d93627b 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -711,8 +711,8 @@ ProcessCopyOptions(ParseState *pstate,
errmsg("COPY force not null available only in CSV mode")));
if (opts_out->force_notnull != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force not null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY force not null cannot be used with COPY TO")));
/* Check force_null */
if (!opts_out->csv_mode && opts_out->force_null != NIL)
@@ -722,22 +722,28 @@ ProcessCopyOptions(ParseState *pstate,
if (opts_out->force_null != NIL && !is_from)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("COPY force null only available using COPY FROM")));
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY force null cannot be used with COPY TO")));
/* Don't allow the delimiter to appear in the null string. */
if (strchr(opts_out->null_print, opts_out->delim[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("COPY delimiter must not appear in the NULL specification")));
/* Don't allow the CSV quote char to appear in the null string. */
if (opts_out->csv_mode &&
strchr(opts_out->null_print, opts_out->quote[0]) != NULL)
ereport(ERROR,
-(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("CSV quote character must not appear in the NULL specification")));
+ /* Check freeze */
+ if (opts_out->freeze && !is_from)
+ ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("COPY FREEZE mode cannot be used with COPY FROM")));
+
if (opts_out->default_print)
{
if (!is_from)
diff --git a/src/test/regress/expected/copy2.out b/src/test/regress/expected/copy2.out
index 95ec7363af..98ef2ef140 100644
--- a/src/test/regress/expected/copy2.out
+++ b/src/test/regress/expected/copy2.out
@@ -89,11 +89,11 @@ ERROR: COPY force quote only available using COPY TO
COPY x to stdout (format TEXT, force_not_null(a));
ERROR: COPY force not null available only in CSV mode
COPY x to stdin (format CSV, force_not_null(a));
-ERROR: COPY force not null only available using COPY FROM
+ERROR: COPY force not null cannot be used with COPY TO
COPY x to stdout (format TEXT, force_null(a));
ERROR: COPY force null available only in CSV mode
COPY x to stdin (format CSV, force_null(a));
-ERROR: COPY force null only available using COPY FROM
+ERROR: COPY force null cannot be used with COPY TO
-- too many columns in column list: should fail
COPY x (a, b, c, d, e, d, c) from stdin;
ERROR: column "d" specified more than once
Re: pg_resetwal tests, logging, and docs update
On 29.09.23 10:02, Peter Eisentraut wrote:
On 26.09.23 17:19, Aleksander Alekseev wrote:
Attached is an updated patch set where I have split the changes into
smaller pieces. The last two patches still have some open questions
about what certain constants mean etc. The other patches should be
settled.
The patches 0001..0005 seem to be ready and rather independent. I
suggest merging them and continue discussing the rest of the patches.
I have committed 0001..0005, and also posted a separate patch to discuss
and correct the behavior of the -c option. I expect that we will carry
over this patch set to the next commit fest.
Here are updated versions of the remaining patches. I took out the
"FIXME" notes about the multipliers applying to the -c option and
replaced them by gentler comments. I don't intend to resolve those
issues here.From 16affb34b3f53b4639a136f9eb1d927c89780ec1 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sun, 29 Oct 2023 16:48:16 +0100
Subject: [PATCH v3 1/2] doc: pg_resetwal: Add comments how the multipliers are
derived
Discussion:
https://www.postgresql.org/message-id/flat/0f3ab4a1-ae80-56e8-3426-6b4a02507...@eisentraut.org
---
doc/src/sgml/ref/pg_resetwal.sgml | 8 +++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/pg_resetwal.sgml
b/doc/src/sgml/ref/pg_resetwal.sgml
index 08cd3ce5fc2..cf9c7e70f27 100644
--- a/doc/src/sgml/ref/pg_resetwal.sgml
+++ b/doc/src/sgml/ref/pg_resetwal.sgml
@@ -166,7 +166,8 @@ Options
pg_resetwal is unable to determine appropriate values
by reading pg_control. Safe values can be determined
as
described below. For values that take numeric arguments, hexadecimal
- values can be specified by using the prefix 0x.
+ values can be specified by using the prefix 0x. Note
+ that these instructions only apply with the standard block size of 8 kB.
@@ -189,6 +190,7 @@ Options
greatest file name in the same directory. The file names are in
hexadecimal.
+
@@ -272,6 +274,7 @@ Options
names are in hexadecimal, so the easiest way to do this is to specify
the option value in hexadecimal and append four zeroes.
+
@@ -306,6 +309,7 @@ Options
The file names are in hexadecimal. There is no simple recipe such as
the ones for other options of appending zeroes.
+
@@ -354,6 +358,7 @@ Options
in pg_xact, -u 0x70 will
work (five
trailing zeroes provide the proper multiplier).
+
@@ -375,6 +380,7 @@ Options
in pg_xact, -x 0x120 will
work (five
trailing zeroes provide the proper multiplier).
+
base-commit: 237f8765dfd9149471d37f3754d15cef888338a8
--
2.42.0
From 3b5c60dc87f4440aa81d50d0c6363aea7a970347 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sun, 29 Oct 2023 16:48:16 +0100
Subject: [PATCH v3 2/2] pg_resetwal: Add more tests and test coverage
Discussion:
https://www.postgresql.org/message-id/flat/0f3ab4a1-ae80-56e8-3426-6b4a02507...@eisentraut.org
---
src/bin/pg_resetwal/t/001_basic.pl | 120 +
src/bin/pg_resetwal/t/002_corrupted.pl | 4 +
2 files changed, 124 insertions(+)
diff --git a/src/bin/pg_resetwal/t/001_basic.pl
b/src/bin/pg_resetwal/t/001_basic.pl
index 7e5efbf56b5..6c02c4ae74a 100644
--- a/src/bin/pg_resetwal/t/001_basic.pl
+++ b/src/bin/pg_resetwal/t/001_basic.pl
@@ -14,6 +14,7 @@
my $node = PostgreSQL::Test::Cluster->new('main');
$node->init;
+$node->append_conf('postgresql.conf', 'track_commit_timestamp = on');
command_like([ 'pg_resetwal', '-n', $node->data_dir ],
qr/checkpoint/, 'pg_resetwal -n produces output');
@@ -29,4 +30,123 @@
'check PGDATA permissions');
}
+command_ok([ 'pg_resetwal', '-D', $node->data_dir ], 'pg_resetwal runs');
+$node->start;
+is($node->safe_psql("postgres", "SELECT 1;"), 1, 'server running and working
after reset');
+
+command_fails_like([ 'pg_resetwal', $node->data_dir ], qr/lock file .*
exists/, 'fails if server running');
+
+$node->stop('immediate');
+command_fails_like([ 'pg_resetwal', $node->data_dir ], qr/database server was
not shut down cleanly/, 'does not run after immediate shutdown');
+command_ok([ 'pg_resetwal', '-f', $node->data_dir ], 'runs after immediate
shutdown with force');
+$node->start;
+is($node->safe_psql("postgres", "SELECT 1;"), 1, 'server running and working
after forced reset');
+
+$node->stop;
+
+# check various command-line handling
+
+# Note: This test intends to check that a nonexistent data directory
+# gives a reasonable error message. Because of the way the code is
+# currently structured, you get an error about readings permissions,
+# which is perhaps suboptimal, so feel free to update this test if
+# this gets improved.
+command_fails_like([ 'pg_resetwal', 'foo' ], qr/error: could not read
permissions of directory/,
Re: Supporting MERGE on updatable views
On Sat, 28 Oct 2023 at 09:35, jian he wrote: > > hi. > Excellent work! regress test passed! The code looks so intuitive! > Thanks for looking! > doc/src/sgml/ref/create_view.sgml. > Do we need to add> If the view or any of its base > relations has an INSTEAD rule that causes the > INSERT or UPDATE command > to be rewritten, then > all check options will be ignored in the rewritten query, including any > checks from automatically updatable views defined on top of the relation > with the INSTEAD rule. > We don't want to include MERGE in that sentence, because MERGE isn't supported on views or tables with rules, but I guess we could add another sentence after that one, to make that clear. > in src/backend/executor/nodeModifyTable.c line 3800: ExecModifyTable > ` > datum = ExecGetJunkAttribute(slot,resultRelInfo->ri_RowIdAttNo,); > . > oldtupdata.t_data = DatumGetHeapTupleHeader(datum); > oldtupdata.t_len = HeapTupleHeaderGetDatumLength(oldtupdata.t_data); > ` > In ExecGetJunkAttribute(slot,resultRelInfo->ri_RowIdAttNo,); > > does resultRelInfo->ri_RowIdAttNo must be 1 to make sure > DatumGetHeapTupleHeader(datum) works? > (I am not familiar with this part.) Well, it's not necessarily 1. It's whatever the attribute number of the "wholerow" attribute is, which can vary. "datum" is then set to the value of the "wholerow" attribute, which is the OLD tuple, so using DatumGetHeapTupleHeader() makes sense. This relies on the code in ExecInitModifyTable(), which sets up ri_RowIdAttNo. Regards, Dean
Re: pg_dump not dumping the run_as_owner setting from version 16?
Philip Warner writes: > Please find attached a patch for pg_dump to honour the setting of > `run_as_owner`; I believe that effective pre-16 behavious was to run as > owner, so I have set the flag to ‘t’ for pre-16 versions. Please let me know > if you would prefer the opposite. I think that's the correct choice. Fix pushed, thanks. regards, tom lane
Re: POC, WIP: OR-clause support for indexes
Hi! On 27.10.2023 00:04, Peter Geoghegan wrote: On Thu, Oct 26, 2023 at 12:59 PM Robert Haas wrote: Alexander's example seems to show that it's not that simple. If I'm reading his example correctly, with things like aid = 1, the transformation usually wins even if the number of things in the OR expression is large, but with things like aid + 1 * bid = 1, the transformation seems to lose at least with larger numbers of items. So it's not JUST the number of OR elements but also what they contain, unless I'm misunderstanding his point. Alexander said "Generally, I don't see why ANY could be executed slower than the equivalent OR clause". I understood that this was his way of expressing the following idea: "In principle, there is no reason to expect execution of ANY() to be slower than execution of an equivalent OR clause (except for noise-level differences). While it might not actually look that way for every single type of plan you can imagine right now, that doesn't argue for making a cost-based decision. It actually argues for fixing the underlying issue, which can't possibly be due to some kind of fundamental advantage enjoyed by expression evaluation with ORs". This is also what I think of all this. Alexander's partial index example had this quality to it. Obviously, the planner *could* be taught to do the right thing with such a case, with a little more work. The fact that it doesn't right now is definitely a problem, and should probably be treated as a blocker for this patch. But that doesn't really argue against the general idea behind the patch -- it just argues for fixing that one problem. There may also be a separate problem that comes from the added planner cycles required to do the transformation -- particularly in extreme or adversarial cases. We should worry about that, too. But, again, it doesn't change the basic fact, which is that having a standard/normalized representation of OR lists/DNF transformation is extremely useful in general, and *shouldn't* result in any real slowdowns at execution time if done well. I think it would be more correct to finalize the current approach to converting "OR" expressions to "ANY", since quite a few problems related to this patch have already been found here, I think you can solve them first, and then you can move on. To me, this sort of output suggests that perhaps the transformation is being done in the wrong place. I expect that we have to decide whether to convert from OR to = ANY(...) at a very early stage of the planner, before we have any idea what the selected path will ultimately be. But this output suggests that we want the answer to depend on which kind of path is going to be faster, which would seem to argue for doing this sort of transformation as part of path generation for only those paths that will benefit from it, rather than during earlier phases of expression processing/simplification. I don't think that that's the right direction. They're semantically equivalent things. But a SAOP-based plan can be fundamentally better, since SAOPs enable passing down useful context to index AMs (at least nbtree). And because we can use a hash table for SAOP expression evaluation. It's a higher level, standardized, well optimized way of expressing exactly the same concept. I can come up with a case that'll be orders of magnitude more efficient with this patch, despite the transformation process only affecting a small OR list of 3 or 5 elements -- a 100x reduction in heap page accesses is quite possible. This is particularly likely to come up if you assume that the nbtree patch that I'm currently working on is also available. In general, I think that we totally over-rely on bitmap index scans, especially BitmapOrs. Regarding the application of the transformation at an early stage, the patch is almost ready, except for solving cases related to queries that work slower. I haven't figured out how to exclude such requests without comparing the cost or parameter by the number of OR elements yet. The simplest option is not to process Expr types (already mentioned earlier) in the queries that Alexander gave as an example, but as I already said, I don't like this approach very much. -- Regards, Alena Rybakina Postgres Professional
Re: Use virtual tuple slot for Unique node
I have taken a look at this discussion, at the code and I am confused how we choose tuple table slot (TTS) type in PG. May be you can clarify this topic or me. 1. Brief intro. There are four types of TTS. Plan tree «leaves»: - buffer heap (produced by index and table scans, has system columns and keeps shared buffer pins) - heap (produced by FDW: has system columns, but doesn’t keep any pins) - minimal (produced by values and materializations nodes like sort, agg, etc.) Plan «branches»: - virtual (keeps datum references to the columns of the tuples in the child nodes) Virtual TTS is cheeper to copy among the plan (as we copy datum references), but more expensive to materialize (we have to construct a tuple from pieces). Leaves are cheeper to materialize (as we make a memcmp under hood), but very expensive to copy (we copy the value, not the datum reference). 2. If we take a look at the materialize nodes in the plan, they produce different result TTS. - Outer child TTS type: gater, gather merge, lock rows, limit; - Minimal: material, sort, incremental sort, memoize, unique, hash, setup (can be heap as well); - Virtual: group, agg, window agg. From my point of view, the materialization node should preserve the incoming TTS type. For the sort node (that materializes incoming tuples as minimal) it is ok to output minimal result as well. Looks that unique should use the outer child’d TTS (instead of hardcoded minimal). But can anyone explain me why do group, agg and window agg return the virtual instead of the same TTS type as outer child has? Do we expect that the parent node exists and requires exactly virtual tuples (but what if the parent node is sort and benefits from minimal TTS)? So, it looks like we need to take a look not only at the unique, but also inspect all the materialization nodes.
Re: On login trigger: take three
Mikhail Gribkov writes: > Just for a more complete picture of the final state here. > I have posted the described fix (for avoiding race condition in the tests) > separately: > https://commitfest.postgresql.org/45/4616/ It turns out that the TAP test for this feature (006_login_trigger.pl) also has a race condition: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=mamba=2023-10-28%2003%3A33%3A28 The critical bit of the log is ack Broken pipe: write( 14, 'SELECT 1;' ) at /usr/pkg/lib/perl5/vendor_perl/5.36.0/IPC/Run/IO.pm line 550. It looks to me that what happened here is that the backend completed the authentication handshake, and then the login trigger caused a FATAL exit, and after than the connected psql session tried to send "SELECT 1" on an already-closed pipe. That failed, causing IPC::Run to panic. mamba is a fairly slow machine and doubtless has timing a bit different from what this test was created on. But I doubt there is any way to make this behavior perfectly stable across a range of machines, so I recommend just removing the test case involving a fatal exit. regards, tom lane
Re: Issues with Information_schema.views
Erki Eessaar writes: > My question is - is all of this the intended behaviour by the implementers? Yes, I'd say so. If you are expecting that the is_updatable flag will check to see if the behavior provided by the view's rules corresponds to something that a human would call a corresponding update of the view's output, you're out of luck. There's a little issue called the halting problem. So the actual check just looks to see if there's unconditional DO INSTEAD rules of the appropriate types, and doesn't probe into what those rules do. regards, tom lane
Re: run pgindent on a regular basis / scripted manner
On 2023-10-28 Sa 12:09, Tom Lane wrote: Andrew Dunstan writes: Based on recent experience, where a lot koel's recent complaints seem to be about comments, I'd like to suggest a modest adjustment. First, we should provide a mode of pgindent that doesn't reflow comments. pg_bsd_indent has a flag for this (-nfcb), so this should be relatively simple. Second, koel could use that mode, so that it wouldn't complain about comments it thinks need to be reflowed. Of course, we'd fix these up with our regular pgindent runs. Seems like a bit of a kluge. Maybe it's the right thing to do, but I don't think we have enough data points yet to be confident that it'd meaningfully reduce the number of breakages. On a more abstract level: the point of trying to maintain indent cleanliness is so that if you modify a file and then want to run pgindent on your own changes, you don't get incidental changes elsewhere in the file. This solution would break that, so I'm not sure it isn't throwing the baby out with the bathwater. Yeah, could be. cheers andrew. -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: Issues with Information_schema.views
On Sun, Oct 29, 2023 at 4:05 PM Erki Eessaar wrote: > > Hello > > Thank you! I know that. > > > Secondly, the rule you demonstrated does not alone change IS_UPDATABLE value > to YES. I have to create two rules: > > CREATE OR REPLACE RULE emps_update AS ON UPDATE > TO Emps > DO INSTEAD UPDATE emp SET > empno = NEW.empno, > ename = NEW.ename, > deptno = NEW.deptno; > > CREATE OR REPLACE RULE emps_delete AS ON DELETE > TO Emps > DO INSTEAD DELETE FROM Emp WHERE empno=OLD.empno; > > My question is - is all of this the intended behaviour by the implementers? > > Best regards > Erki Eessaar > per test, it's the expected behavior. https://git.postgresql.org/cgit/postgresql.git/tree/src/test/regress/expected/updatable_views.out#n569 https://git.postgresql.org/cgit/postgresql.git/tree/src/test/regress/expected/updatable_views.out#n603 https://git.postgresql.org/cgit/postgresql.git/tree/src/test/regress/expected/updatable_views.out#n637 you need CREATE RULE AS ON DELETE and CREATE RULE AS ON UPDATE to mark the view as is_updatable.
Re: Use virtual tuple slot for Unique node
On Fri, 27 Oct 2023 at 22:05, Ashutosh Bapat wrote: > > On Fri, Oct 27, 2023 at 8:48 AM David Rowley wrote: > > I was uncertain if the old behaviour of when srcslot contains fewer > > attributes than dstslot was intended or not. What happens there is > > that we'd leave the additional old dstslot tts_values in place and > > only overwrite up to srcslot->natts but then we'd go on and > > materialize all the dstslot attributes. I think this might not be > > needed as we do dstslot->tts_nvalid = srcdesc->natts. I suspect we may > > be ok just to materialize the srcslot attributes and ignore the > > previous additional dstslot attributes. Changing it to that would > > make the attached patch more simple. > > We seem to use both tt_nvalid and tts_tupleDescriptor->natts. I forgot > what's the difference. If we do what you say, we might end up trying > to access unmaterialized values beyond tts_nvalid. Better to > investigate whether such a hazard exists. The TupleDesc's natts is the number of attributes in the tuple descriptor. tts_nvalid is the greatest attribute number that's been deformed in the tuple slot. For slot types other than virtual slots, we'll call slot_getsomeattrs() to deform more attributes from the tuple. The reason the code in question looks suspicious to me is that we do "dstslot->tts_nvalid = srcdesc->natts;" and there's no way to deform more attributes in a virtual slot. Note that tts_virtual_getsomeattrs() unconditionally does elog(ERROR, "getsomeattrs is not required to be called on a virtual tuple table slot");. We shouldn't ever be accessing tts_values elements above what tts_nvalid is set to, so either we should be setting dstslot->tts_nvalid = to the dstdesc->natts so that we can access tts_values elements above srcdesc->natts or we're needlessly materialising too many attributes in tts_virtual_copyslot(). David David
Re: Issues with Information_schema.views
Hello Thank you! I know that. DO INSTEAD NOTHING rules on updatable views could be used as a way to implement WITH READ ONLY constraint (one can define such constraint in Oracle). However, one could accidentally add such rule to non-updatable view as well. I tried to construct a system-catalog based query to find database rules that are unnecessary. Thus, for the testing purposes I added a DO INSTEAD NOTHING rule to already non-updatable view and was a bit surprised that INFORMATION_SCHEMA-based check showed that the view had become updatable. A possible reasoning is that I can update the view without getting an error. However, I still cannot change data in base tables. Secondly, the rule you demonstrated does not alone change IS_UPDATABLE value to YES. I have to create two rules: CREATE OR REPLACE RULE emps_update AS ON UPDATE TO Emps DO INSTEAD UPDATE emp SET empno = NEW.empno, ename = NEW.ename, deptno = NEW.deptno; CREATE OR REPLACE RULE emps_delete AS ON DELETE TO Emps DO INSTEAD DELETE FROM Emp WHERE empno=OLD.empno; My question is - is all of this the intended behaviour by the implementers? Best regards Erki Eessaar From: jian he Sent: Saturday, October 28, 2023 13:38 To: Erki Eessaar Cc: pgsql-hackers@lists.postgresql.org Subject: Re: Issues with Information_schema.views On Sat, Oct 28, 2023 at 5:27 PM Erki Eessaar wrote: > > Hello > > > /*After that: is_updatable=YES*/ > > 1. Indeed, now I can execute INSERT/UPDATE/DELETE against the view without > getting an error. However, I still cannot change the data in the database > through the views. https://www.postgresql.org/docs/current/sql-createview.html " A more complex view that does not satisfy all these conditions is read-only by default: the system will not allow an insert, update, or delete on the view. You can get the effect of an updatable view by creating INSTEAD OF triggers on the view, which must convert attempted inserts, etc. on the view into appropriate actions on other tables. For more information see CREATE TRIGGER. Another possibility is to create rules (see CREATE RULE), but in practice triggers are easier to understand and use correctly. " You CAN get the effect of an updateable view. But you need to make the rule/triggers correct. the following RULE can get the expected result. CREATE OR REPLACE RULE emps_update AS ON UPDATE TO Emps DO INSTEAD UPDATE emp SET empno = NEW.empno, ename = NEW.ename, deptno = NEW.deptno; you can also look at src/test/regress/sql/triggers.sql, src/test/regress/sql/rules.sql for more test cases.
Re: COPY TO (FREEZE)?
Mingli Zhang 于2023年10月29日 周日14:35写道：
>
>
> Bruce Momjian 于2023年10月29日 周日10:04写道：
>
>> On Sat, Oct 28, 2023 at 09:54:05PM -0400, Tom Lane wrote:
>> > Bruce Momjian writes:
>> > > On Sat, Oct 28, 2023 at 09:39:53PM -0400, Tom Lane wrote:
>> > >> Not thrilled by the wording here.
>> >
>> > > I think it is modeled after:
>> >
>> > > errmsg("COPY force null only available using COPY FROM")));
>> >
>> > Well, now that you bring it up, that's no sterling example of
>> > clear writing either. Maybe change that while we're at it,
>> > say to "FORCE NULL option must not be used in COPY TO"?
>>
>> I used:
>>
>> "COPY FREEZE mode cannot be used with COPY FROM"
>>
>> and adjusted the others.
>>
>> > (Also, has it got the right ERRCODE?)
>>
>> Fixed, and the other cases too. Patch attached.
>>
>> --
>> Bruce Momjian https://momjian.us
>> EDB https://enterprisedb.com
>>
>> Only you can decide what is important to you.
>
>
> errmsg("COPY force not null only available using COPY FROM")));
>>
>> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>>
>> + errmsg("COPY force not null cannot be used with COPY FROM")));
>>
>>
> cannot -> can ?
>
I guess you want to write “cannot be used with COPY TO”
>
Re: COPY TO (FREEZE)?
Bruce Momjian 于2023年10月29日 周日10:04写道：
> On Sat, Oct 28, 2023 at 09:54:05PM -0400, Tom Lane wrote:
> > Bruce Momjian writes:
> > > On Sat, Oct 28, 2023 at 09:39:53PM -0400, Tom Lane wrote:
> > >> Not thrilled by the wording here.
> >
> > > I think it is modeled after:
> >
> > > errmsg("COPY force null only available using COPY FROM")));
> >
> > Well, now that you bring it up, that's no sterling example of
> > clear writing either. Maybe change that while we're at it,
> > say to "FORCE NULL option must not be used in COPY TO"?
>
> I used:
>
> "COPY FREEZE mode cannot be used with COPY FROM"
>
> and adjusted the others.
>
> > (Also, has it got the right ERRCODE?)
>
> Fixed, and the other cases too. Patch attached.
>
> --
> Bruce Momjian https://momjian.us
> EDB https://enterprisedb.com
>
> Only you can decide what is important to you.
errmsg("COPY force not null only available using COPY FROM")));
>
> + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
>
> + errmsg("COPY force not null cannot be used with COPY FROM")));
>
>
cannot -> can ?
>
