Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

thub.com/michaelpq/pg_plugins/tree/main/blackhole_am
> * perf is used but used options are unknown (sorry)
>
> (*1) This SQL may be used to create the table:
>
> CREATE OR REPLACE FUNCTION create_table_cols(tabname text, num_cols int)
> RETURNS VOID AS
> $func$
> DECLARE
>   query text;
> BEGIN
>   query := 'CREATE UNLOGGED TABLE ' || tabname || ' (';
>   FOR i IN 1..num_cols LOOP
> query := query || 'a_' || i::text || ' int default 1';
> IF i != num_cols THEN
>   query := query || ', ';
> END IF;
>   END LOOP;
>   query := query || ')';
>   EXECUTE format(query);
> END
> $func$ LANGUAGE plpgsql;
> SELECT create_table_cols ('to_tab_30', 30);
> SELECT create_table_cols ('from_tab_30', 30);
>
> (*2) This SQL may be used to insert 5M rows:
>
> INSERT INTO to_tab_30 SELECT FROM generate_series(1, 500);
>
> (*3) This SQL may be used for COPY TO:
>
> COPY to_tab_30 TO '/dev/null' WITH (FORMAT text);
>
> (*4) This SQL may be used for COPY FROM:
>
> CREATE EXTENSION blackhole_am;
> ALTER TABLE from_tab_30 SET ACCESS METHOD blackhole_am;
> COPY to_tab_30 TO '/tmp/to_tab_30.txt' WITH (FORMAT text);
> COPY from_tab_30 FROM '/tmp/to_tab_30.txt' WITH (FORMAT text);
>
>
> If there is enough information, could you try?
>
Thanks for updating the patches, I applied them and test
in my local machine, I did not use tmpfs in my test, I guess
if I run the tests enough rounds, the OS will cache the
pages, below is my numbers(I run each test 30 times, I
count for the last 10 ones):

HEADPATCHED

COPY to_tab_30 TO '/dev/null' WITH (FORMAT text);

5628.280 ms   5679.860 ms
5583.144 ms   5588.078 ms
5604.444 ms   5628.029 ms
5617.133 ms   5613.926 ms
5575.570 ms   5601.045 ms
5634.828 ms   5616.409 ms
5693.489 ms   5637.434 ms
5585.857 ms   5609.531 ms
5613.948 ms   5643.629 ms
5610.394 ms   5580.206 ms

COPY from_tab_30 FROM '/tmp/to_tab_30.txt' WITH (FORMAT text);

3929.955 ms   4050.895 ms
3909.061 ms   3890.156 ms
3940.272 ms   3927.614 ms
3907.535 ms   3925.560 ms
3952.719 ms   3942.141 ms
3933.751 ms   3904.250 ms
3958.274 ms   4025.581 ms
3937.065 ms   3894.149 ms
3949.896 ms   3933.878 ms
3925.399 ms   3936.170 ms

I did not see obvious performance degradation, maybe it's
because I did not use tmpfs, but I think this OTH means
that the *function call* and *if branch* added for each row
is not the bottleneck of the whole execution path.

In 0001,

+typedef struct CopyFromRoutine
+{
+ /*
+ * Called when COPY FROM is started to set up the input functions
+ * associated to the relation's attributes writing to.  `finfo` can be
+ * optionally filled to provide the catalog information of the input
+ * function.  `typioparam` can be optionally filled to define the OID of
+ * the type to pass to the input function.  `atttypid` is the OID of data
+ * type used by the relation's attribute.

+typedef struct CopyToRoutine
+{
+ /*
+ * Called when COPY TO is started to set up the output functions
+ * associated to the relation's attributes reading from.  `finfo` can be
+ * optionally filled.  `atttypid` is the OID of data type used by the
+ * relation's attribute.

The second comment has a simplified description for `finfo`, I think it
should match the first by:

`finfo` can be optionally filled to provide the catalog information of the
output function.

After I post the patch diffs, the gmail grammer shows some hints that
it should be *associated with* rather than *associated to*, but I'm
not sure about this one.

I think the patches are in good shape, I can help to do some
further tests if needed, thanks for working on this.

>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao

Re: Extension using Meson as build system

[Junwang Zhao]

Hi, Peter

On Fri, Jul 26, 2024 at 11:06 PM Peter Eisentraut  wrote:
>
> On 30.06.24 15:17, Junwang Zhao wrote:
> > Is there any extension that uses meson as build systems?
> > I'm starting a extension project that written in c++, cmake is my
> > initial choice as the build system, but since PostgreSQL has adopted
> > Meson, so I'm wondering if there is any extension that also uses
> > meson that I can reference.
>
> I wrote a little demo:
>
> https://github.com/petere/plsh/blob/meson/meson.build
>

Thanks, I will check it out ;)

-- 
Regards
Junwang Zhao

Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails

[Junwang Zhao]

On Fri, Jul 26, 2024 at 2:36 PM Junwang Zhao  wrote:
>
> On Mon, Jul 22, 2024 at 1:52 PM Tender Wang  wrote:
> >
> >
> >
> > Alvaro Herrera  于2024年7月19日周五 21:18写道：
> >>
> >> Hello,
> >>
> >> I think the fix for the check triggers should be as the attached.  Very
> >> close to what you did, but you were skipping some operations that needed
> >> to be kept.  AFAICS this patch works correctly for the posted cases.
> >
> >
> > After applying the attached, the r_1_p_id_fkey1 will have redundant action
> > triggers, as below:
> > postgres=# select oid, conname, contype, conrelid, conindid,conparentid, 
> > confrelid,conislocal,coninhcount, connoinherit from pg_constraint where oid 
> > = 16402;
> >   oid  |conname | contype | conrelid | conindid | conparentid | 
> > confrelid | conislocal | coninhcount | connoinherit
> > ---++-+--+--+-+---++-+--
> >  16402 | r_1_p_id_fkey1 | f   |16394 |16392 |   0 | 
> > 16389 | t  |   0 | f
> > (1 row)
> >
> > postgres=# select oid, tgrelid, tgparentid, tgconstrrelid, tgconstrindid, 
> > tgconstraint from pg_trigger where tgconstraint = 16402;
> >   oid  | tgrelid | tgparentid | tgconstrrelid | tgconstrindid | tgconstraint
> > ---+-++---+---+--
> >  16403 |   16389 |  16400 | 16394 | 16392 |16402
> >  16404 |   16389 |  16401 | 16394 | 16392 |16402
> >  16422 |   16389 |  0 | 16394 | 16392 |16402
> >  16423 |   16389 |  0 | 16394 | 16392 |16402
> > (4 rows)
> >
>
> Yes, seems Alvaro has mentioned that he hasn't looked at the
> action triggers, in the attached patch, I add some logic that
> first check if there exists action triggers, if yes, just update
> their Parent Trigger to InvalidOid.
>
> >
> > --
> > Tender Wang
>
>
>
> --
> Regards
> Junwang Zhao

There is a bug report[0] Tender comments might be the same
issue as this one, but I tried Alvaro's and mine patch, neither
could solve that problem, I did not tried Tender's earlier patch
thought. I post the test script below in case you are interested.

CREATE TABLE t1 (a int, PRIMARY KEY (a));
CREATE TABLE t (a int, PRIMARY KEY (a), FOREIGN KEY (a) REFERENCES t1)
PARTITION BY LIST (a);
ALTER TABLE t ATTACH PARTITION t1 FOR VALUES IN (1);
ALTER TABLE t DETACH PARTITION t1;
ALTER TABLE t ATTACH PARTITION t1 FOR VALUES IN (1);


[0] https://www.postgresql.org/message-id/18541-628a61bc267cd...@postgresql.org

-- 
Regards
Junwang Zhao

Re: [BUG] Fix DETACH with FK pointing to a partitioned table fails

[Junwang Zhao]

On Mon, Jul 22, 2024 at 1:52 PM Tender Wang  wrote:
>
>
>
> Alvaro Herrera  于2024年7月19日周五 21:18写道：
>>
>> Hello,
>>
>> I think the fix for the check triggers should be as the attached.  Very
>> close to what you did, but you were skipping some operations that needed
>> to be kept.  AFAICS this patch works correctly for the posted cases.
>
>
> After applying the attached, the r_1_p_id_fkey1 will have redundant action
> triggers, as below:
> postgres=# select oid, conname, contype, conrelid, conindid,conparentid, 
> confrelid,conislocal,coninhcount, connoinherit from pg_constraint where oid = 
> 16402;
>   oid  |conname | contype | conrelid | conindid | conparentid | 
> confrelid | conislocal | coninhcount | connoinherit
> ---++-+--+--+-+---++-+--
>  16402 | r_1_p_id_fkey1 | f   |16394 |16392 |   0 | 
> 16389 | t  |   0 | f
> (1 row)
>
> postgres=# select oid, tgrelid, tgparentid, tgconstrrelid, tgconstrindid, 
> tgconstraint from pg_trigger where tgconstraint = 16402;
>   oid  | tgrelid | tgparentid | tgconstrrelid | tgconstrindid | tgconstraint
> ---+-++---+---+--
>  16403 |   16389 |  16400 | 16394 | 16392 |16402
>  16404 |   16389 |  16401 | 16394 | 16392 |16402
>  16422 |   16389 |  0 | 16394 | 16392 |16402
>  16423 |   16389 |  0 | 16394 | 16392 |16402
> (4 rows)
>

Yes, seems Alvaro has mentioned that he hasn't looked at the
action triggers, in the attached patch, I add some logic that
first check if there exists action triggers, if yes, just update
their Parent Trigger to InvalidOid.

>
> --
> Tender Wang



-- 
Regards
Junwang Zhao


v2-0001-Fix-partition-detach-on-tables-with-FKs-to-partit.patch
Description: Binary data

Re: Add new COPY option REJECT_LIMIT

[Junwang Zhao]

Hi Torikoshia,

On Wed, Jul 17, 2024 at 9:21 PM torikoshia  wrote:
>
> On 2024-07-03 02:07, Fujii Masao wrote:
> > However, if we support REJECT_LIMIT, I'm not sure if the ON_ERROR
> > option is still necessary.
>
> I remembered another reason for the necessity of ON_ERROR.
>
> ON_ERROR defines how to behave when encountering an error and it just
> accepts 'ignore' and 'stop' currently, but is expected to support other
> options such as saving details of errors to a table[1].
> ON_ERROR=stop is a synonym for REJECT_LIMIT=infinity, but I imagine
> REJECT_LIMIT would not replace future options of ON_ERROR.
>
> Considering this and the option we want to add this time is to specify
> an upper limit on the number or ratio of errors, the name of this option
> like "reject_limit" seems better than "ignore_errors".
>
> On Fri, Jul 5, 2024 at 4:13 PM torikoshia 
> wrote:
> > On 2024-07-05 12:59, Fujii Masao wrote:
> >> On 2024/07/04 12:05, torikoshia wrote:
> >>> I'm going to update it after discussing the option format as
> >>> described
> >>> below.
>
> Updated the patch.
> 0001 sets limit by the absolute number of error rows and 0002 sets limit
> by ratio of the error.

In patch 0002, the ratio is calculated by the already skipped/processed
rows, but what if a user wants to copy 1000 rows, and he/she can tolerate
10 error rows, so he/she might set *reject_limit 0.01*, but one bad row in the
first 100 rows will fail the entire command, this might surprise the user.

This case can be resolved by 0001 *reject_limit 10*, so I think the *by ratio*
is less useful.

>
> >> If we choose "all" as the keyword, renaming the option to
> >> IGNORE_ERRORS
> >> might be more intuitive and easier to understand than REJECT_LIMIT.
>
> > I feel that 'infinite' and 'unlimited' are unfamiliar values for
> > PostgreSQL parameters, so 'all' might be better and IGNORE_ERRORS would
> > be a better parameter name as your suggestion.
>
> As described above, attached patch adopts REJECT_LIMIT, so it uses
> "infinity".
>
> >> This makes me think it might be better to treat REJECT_LIMIT as
> >> an additional option for ON_ERROR=stop instead of ON_ERROR=ignore
> >> if we adopt your patch. Since ON_ERROR=stop is the default,
> >> users could set the maximum number of allowed errors by specifying
> >> only REJECT_LIMIT. Otherwise, they would need to specify both
> >> ON_ERROR=ignore and REJECT_LIMIT.
>
> > That makes sense.
>
> On my second thought, whatever value ON_ERROR is specified(e.g. ignore,
> stop, table), it seems fine to use REJECT_LIMIT.
> I feel REJECT_LIMIT has both "ignore" and "stop" characteristics,
> meaning it ignores errors until it reaches REJECT_LIMIT and stops when
> it exceeds the REJECT_LIMIT.
> And REJECT_LIMIT seems orthogonal to 'table',  which specifies where to
> save error details.
>
> Attached patch allows using REJECT_LIMIT regardless of the ON_ERROR
> option value.
>
>
> [1]
> https://www.postgresql.org/message-id/flat/CACJufxH_OJpVra=0c4ow8fbxhj7hemcvatnepa5vaursena...@mail.gmail.com
>
> --
> Regards,
>
> --
> Atsushi Torikoshi
> NTT DATA Group Corporation



-- 
Regards
Junwang Zhao

Support specify tablespace for each merged/split partition

[Junwang Zhao]

In [1], it is suggested that it might be a good idea to support
specifying the tablespace for each merged/split partition.

We can do the following after this feature is supported:

CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
CREATE TABLE t (i int PRIMARY KEY) PARTITION BY RANGE (i);
CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);

ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2 TABLESPACE tblspc;

ALTER TABLE t SPLIT PARTITION tp_0_2 INTO
(PARTITION tp_0_1 FOR VALUES FROM (0) TO (1) TABLESPACE tblspc,
PARTITION tp_1_2 FOR VALUES FROM (1) TO (2));

[1] 
https://www.postgresql.org/message-id/abaf390b-3320-40a5-8815-ef476db5c...@oss.nttdata.com

-- 
Regards
Junwang Zhao


0001-support-specify-tablespace-for-each-merged-split.patch
Description: Binary data

Re: MERGE/SPLIT partition commands should create new partitions in the parent's tablespace?

[Junwang Zhao]

On Wed, Jul 10, 2024 at 9:36 PM Masahiko Sawada  wrote:
>
> On Wed, Jul 10, 2024 at 5:14 PM Masahiko Sawada  wrote:
> >
> > On Wed, Jul 10, 2024 at 4:14 PM Fujii Masao  
> > wrote:
> > >
> > >
> > >
> > > On 2024/07/10 12:13, Masahiko Sawada wrote:
> > > > On Sat, Jul 6, 2024 at 4:06 PM Fujii Masao 
> > > >  wrote:
> > > >>
> > > >> Hi,
> > > >>
> > > >> I noticed that ALTER TABLE MERGE PARTITIONS and SPLIT PARTITION 
> > > >> commands
> > > >> always create new partitions in the default tablespace, regardless of
> > > >> the parent's tablespace. However, the indexes of these new partitions 
> > > >> inherit
> > > >> the tablespaces of their parent indexes. This inconsistency seems odd.
> > > >> Is this an oversight or intentional?
> > > >>
> > > >> Here are the steps I used to test this:
> > > >>
> > > >> ---
> > > >> CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
> > > >> CREATE TABLE t (i int PRIMARY KEY USING INDEX TABLESPACE tblspc)
> > > >> PARTITION BY RANGE (i) TABLESPACE tblspc;
> > > >>
> > > >> CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
> > > >> CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
> > > >>
> > > >> ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;
> > > >>
> > > >> SELECT tablename, tablespace FROM pg_tables WHERE tablename IN ('t', 
> > > >> 'tp_0_2') ORDER BY tablename;
> > > >>tablename | tablespace
> > > >> ---+
> > > >>t | tblspc
> > > >>tp_0_2| (null)
> > > >> (2 rows)
> > > >>
> > > >> SELECT indexname, tablespace FROM pg_indexes WHERE tablename IN ('t', 
> > > >> 'tp_0_2') ORDER BY indexname;
> > > >> indexname  | tablespace
> > > >> -+
> > > >>t_pkey  | tblspc
> > > >>tp_0_2_pkey | tblspc
> > > >> ---
> > > >>
> > > >>
> > > >> If it's an oversight, I've attached a patch to ensure these commands 
> > > >> create
> > > >> new partitions in the parent's tablespace.
> > > >
> > > > +1
> > > >
> > > > Since creating a child table through the CREATE TABLE statement sets
> > > > its parent table's tablespace as the child table's tablespace, it is
> > > > logical to set the parent table's tablespace as the merged table's
> > > > tablespace.
>
> One expectation I had for MERGE PARTITION was that if all partition
> tables to be merged are in the same tablespace, the merged table is
> also created in the same tablespace. But it would be an exceptional
> case in a sense, and I agree with the proposed behavior as it's
> consistent. It might be a good idea that we can specify the tablespace
> for each merged/split table in the future.

I agree this is a good idea, so I tried to support this feature.

The attached patch v3-0001 is exactly the same as v2-0001, v3-0002 is
a patch for specifying tablespace for each merged/split table.

I'm not sure this addressed David's concern about the tablespace choice
in ca4103025 though.

>
> BTW the new regression tests don't check the table and index names.
> Isn't it better to show table and index names for better
> diagnosability?
>
> +-- Check the new partition inherits parent's tablespace
> +CREATE TABLE t (i int PRIMARY KEY USING INDEX TABLESPACE regress_tblspace)
> +  PARTITION BY RANGE (i) TABLESPACE regress_tblspace;
> +CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
> +CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
> +ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;
> +SELECT tablespace FROM pg_tables WHERE tablename IN ('t', 'tp_0_2')
> ORDER BY tablespace;
> +tablespace
> +--
> + regress_tblspace
> + regress_tblspace
> +(2 rows)
> +
> +SELECT tablespace FROM pg_indexes WHERE tablename IN ('t', 'tp_0_2')
> ORDER BY tablespace;
> +tablespace
> +--
> + regress_tblspace
> + regress_tblspace
> +(2 rows)
> +
> +DROP TABLE t;
>
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
>
>


-- 
Regards
Junwang Zhao


v3-0002-support-specify-tablespace-for-each-merged-split-.patch
Description: Binary data


v3-0001-Ensure-MERGE-SPLIT-partition-commands-create-new-.patch
Description: Binary data

Re: jsonpath: Inconsistency of timestamp_tz() Output

[Junwang Zhao]

On Tue, Jul 9, 2024 at 11:38 PM David E. Wheeler  wrote:
>
> On Jul 9, 2024, at 11:08, Junwang Zhao  wrote:
>
> > In JsonbValue.val.datatime, there is a tz field, I think that's where
> > the offset stored, it is 18000 in the first example
> >
> > struct
> > {
> > Datum value;
> > Oid typid;
> > int32 typmod;
> > int tz; /* Numeric time zone, in seconds, for
> >  * TimestampTz data type */
> > } datetime;
>
> Oooh, okay, so it’s a jsonb variant of the type. Interesting. Ah, and it’s 
> assigned here[1]:
>
>   jb->val.datetime.tz = tz;
>
> It seems like JSONB timestamptz values want to display the recorded time 
> zone, so I suspect we need to set it when the converting from a non-tz to a 
> local tz setting, something like this:
>
> ``` patch
> diff --git a/src/backend/utils/adt/jsonpath_exec.c 
> b/src/backend/utils/adt/jsonpath_exec.c
> index d79c929822..f63b3b9330 100644
> --- a/src/backend/utils/adt/jsonpath_exec.c
> +++ b/src/backend/utils/adt/jsonpath_exec.c
> @@ -2707,12 +2707,16 @@ executeDateTimeMethod(JsonPathExecContext *cxt, 
> JsonPathItem *jsp,
> break;
> case jpiTimestampTz:
> {
> +   struct pg_tm *tm;
> /* Convert result type to timestamp with time 
> zone */
> switch (typid)
> {
> case DATEOID:
> 
> checkTimezoneIsUsedForCast(cxt->useTz,
>   
>  "date", "timestamptz");
> +   if 
> (timestamp2tm(DatumGetTimestamp(value), NULL, tm, NULL, NULL, NULL) == 0) {
> +   tz = 
> DetermineTimeZoneOffset(tm, session_timezone);
> +   }
> value = 
> DirectFunctionCall1(date_timestamptz,
>   
>   value);
> break;
> @@ -2726,6 +2730,9 @@ executeDateTimeMethod(JsonPathExecContext *cxt, 
> JsonPathItem *jsp,
> case TIMESTAMPOID:
> 
> checkTimezoneIsUsedForCast(cxt->useTz,
>   
>  "timestamp", "timestamptz");
> +   if 
> (timestamp2tm(DatumGetTimestamp(value), NULL, tm, NULL, NULL, NULL) == 0) {
> +   tz = 
> DetermineTimeZoneOffset(tm, session_timezone);
> +   }
> value = 
> DirectFunctionCall1(timestamp_timestamptz,
>   
>   value);
> break;
> ```
>
> Only, you know, doesn’t crash the server.

I apply your patch with some minor change(to make the server not crash):

diff --git a/src/backend/utils/adt/jsonpath_exec.c
b/src/backend/utils/adt/jsonpath_exec.c
index d79c9298227..87a695ef633 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -2708,6 +2708,8 @@ executeDateTimeMethod(JsonPathExecContext *cxt,
JsonPathItem *jsp,
case jpiTimestampTz:
{
/* Convert result type to timestamp
with time zone */
+   struct pg_tm tm;
+   fsec_t fsec;
switch (typid)
{
case DATEOID:
@@ -2726,6 +2728,9 @@ executeDateTimeMethod(JsonPathExecContext *cxt,
JsonPathItem *jsp,
case TIMESTAMPOID:

checkTimezoneIsUsedForCast(cxt->useTz,

"timestamp", "timestamptz");
+   if
(timestamp2tm(DatumGetTimestamp(value), NULL, , , NULL, NULL)
== 0) {
+   tz =
DetermineTimeZoneOffset(, session_timezone);
+   }
value =
DirectFunctionCall1(timestamp_timesta

Re: make pg_ctl more friendly

[Junwang Zhao]

Hi, Laurenz

On Wed, Jul 10, 2024 at 3:59 AM Laurenz Albe  wrote:
>
> On Wed, 2024-01-17 at 17:33 +0800, Junwang Zhao wrote:
> > On Wed, Jan 17, 2024 at 4:54 PM Alvaro Herrera  
> > wrote:
> > > I think this needs more comments.  First, in the WaitPMResult enum, we
> > > currently have three values -- READY, STILL_STARTING, FAILED.  These are
> > > all pretty self-explanatory.  But this patch adds SHUTDOWN_IN_RECOVERY,
> > > and that's not at all self-explanatory.  Did postmaster start or not?
> > > The enum value's name doesn't make that clear.  So I'd do something like
> > >
> > > typedef enum
> > > {
> > > POSTMASTER_READY,
> > > POSTMASTER_STILL_STARTING,
> > > /*
> > >  * postmaster no longer running, because it stopped after recovery
> > >  * completed.
> > >  */
> > > POSTMASTER_SHUTDOWN_IN_RECOVERY,
> > > POSTMASTER_FAILED,
> > > } WaitPMResult;
> > >
> > > Maybe put the comments in wait_for_postmaster_start instead.
> >
> > I put the comments in WaitPMResult since we need to add two
> > of those if in wait_for_postmaster_start.
>
> I don't think that any comment is needed; the name says it all.
>
> > > Secondly, the patch proposes to add new text to be returned by
> > > do_start() when this happens, which would look like this:
> > >
> > >   waiting for server to start...  shut down while in recovery
> > >   update recovery target settings for startup again if needed
> > >
> > > Is this crystal clear?  I'm not sure.  How about something like this?
> > >
> > >   waiting for server to start...  done, but not running
> > >   server shut down because of recovery target settings
> > >
> > > variations on the first phrase:
> > >
> > > "done, no longer running"
> > > "done, but no longer running"
> > > "done, automatically shut down"
> > > "done, automatically shut down after recovery"
> >
> > I chose the last one because it gives information to users.
> > See V5, thanks for the wording ;)
>
> Why not just leave it at plain "done"?
> After all, the server was started successfully.
> The second message should make sufficiently clear that the server has stopped.
>
>
> I didn't like the code duplication introduced by the patch, so I rewrote
> that part a bit.
>
> Attached is my suggestion.

The patch LGTM.

>
> I'll set the status to "waiting for author"; if you are fine with my version,
> I think that the patch is "ready for committer".

I've set it to "ready for committer", thanks.

>
> Yours,
> Laurenz Albe



-- 
Regards
Junwang Zhao

Re: jsonpath: Inconsistency of timestamp_tz() Output

[Junwang Zhao]

On Tue, Jul 9, 2024 at 10:22 PM David E. Wheeler  wrote:
>
> On Jul 9, 2024, at 10:07, David E. Wheeler  wrote:
>
> > So perhaps I had things reversed before. Maybe it’s actually doing the 
> > right then when it converts a timestamp to a timestamptz, but not when it 
> > the input contains an offset, as in your example.
>
> To clarify, there’s an inconsistency in the output of timestamp_tz() 
> depending on whether the input has an offset or not. With offset:
>
> david=# select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', 
> '$.timestamp_tz()');
>  jsonb_path_query_tz
> -
>  "2024-08-15T12:34:56-05:00"
>
> And without:
>
> david=# select jsonb_path_query_tz('"2024-08-15 12:34:56"', 
> '$.timestamp_tz()');
>  jsonb_path_query_tz
> -
>  "2024-08-15T16:34:56+00:00"
>
> I suspect the latter is correct, given that the timestamptz type appears to 
> be an int64, presumably always in UTC. I don’t understand where the first 
> example stores the offset.

In JsonbValue.val.datatime, there is a tz field, I think that's where
the offset stored, it is 18000 in the first example

struct
{
  Datum value;
  Oid typid;
  int32 typmod;
  int tz; /* Numeric time zone, in seconds, for
* TimestampTz data type */
} datetime;

>
> Best,
>
> David
>
>
>


-- 
Regards
Junwang Zhao

Re: Address the -Wuse-after-free warning in ATExecAttachPartition()

[Junwang Zhao]

On Tue, Jul 9, 2024 at 7:39 PM Amit Langote  wrote:
>
> On Tue, Jul 9, 2024 at 19:58 Junwang Zhao  wrote:
>>
>> On Tue, Jul 9, 2024 at 6:18 PM Amit Langote  wrote:
>> >
>> > Hi Junwang,
>> >
>> > On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao  wrote:
>> > > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav
>> > >  wrote:
>> > > >
>> > > > In [1], Andres reported a -Wuse-after-free bug in the
>> > > > ATExecAttachPartition() function.  I've created a patch to address it
>> > > > with pointers from Amit offlist.
>> > > >
>> > > > The issue was that the partBoundConstraint variable was utilized after
>> > > > the list_concat() function. This could potentially lead to accessing
>> > > > the partBoundConstraint variable after its memory has been freed.
>> > > >
>> > > > The issue was resolved by using the return value of the list_concat()
>> > > > function, instead of using the list1 argument of list_concat(). I
>> > > > copied the partBoundConstraint variable to a new variable named
>> > > > partConstraint and used it for the previous references before invoking
>> > > > get_proposed_default_constraint(). I confirmed that the
>> > > > eval_const_expressions(), make_ands_explicit(),
>> > > > map_partition_varattnos(), QueuePartitionConstraintValidation()
>> > > > functions do not modify the memory location pointed to by the
>> > > > partBoundConstraint variable. Therefore, it is safe to use it for the
>> > > > next reference in get_proposed_default_constraint()
>> > > >
>> > > > Attaching the patch. Please review and share the comments if any.
>> > > > Thanks to Andres for spotting the bug and some off-list advice on how
>> > > > to reproduce it.
>> > >
>> > > The patch LGTM.
>> > >
>> > > Curious how to reproduce the bug ;)
>> >
>> > Download and apply (`git am`) Andres' patch to "add allocator
>> > attributes" here (note it's not merged into the tree yet!):
>> > https://github.com/anarazel/postgres/commit/99067d5c944e7bd29a8702689f470f623723f4e7.patch
>> >
>> > Then configure using meson with -Dc_args="-Wuse-after-free=3"
>> > --buildtype=release
>> >
>> > Compiling with gcc-12 or gcc-13 should give you the warning that looks
>> > as follows:
>> >
>> > [713/2349] Compiling C object
>> > src/backend/postgres_lib.a.p/commands_tablecmds.c.o
>> > ../src/backend/commands/tablecmds.c: In function ‘ATExecAttachPartition’:
>> > ../src/backend/commands/tablecmds.c:18593:25: warning: pointer
>> > ‘partBoundConstraint’ may be used after ‘list_concat’
>> > [-Wuse-after-free]
>> > 18593 |
>> > get_proposed_default_constraint(partBoundConstraint);
>> >   |
>> > ^~~~
>> > ../src/backend/commands/tablecmds.c:18546:26: note: call to ‘list_concat’ 
>> > here
>> > 18546 | partConstraint = list_concat(partBoundConstraint,
>> >   |  ^~~~~~~~
>> > 18547 |
>> >   RelationGetPartitionQual(rel));
>> >   |
>> >   ~~
>> >
>> > --
>> > Thanks, Amit Langote
>>
>> Thanks Amit,
>>
>> Good to know.
>>
>> When Nitin says:
>>
>> ```Thanks to Andres for spotting the bug and some off-list advice on how
>> to reproduce it.```
>>
>> I thought maybe there is some test case that can really trigger the
>> use-after-free bug, I might get it wrong though ;)
>
>
> I doubt one could write a regression test to demonstrate the use-after-free 
> bug, though I may of course be wrong. By “reproduce it”, I think Nitin meant 
> the warning that suggests that use-after-free bug may occur.

got it, thanks~

-- 
Regards
Junwang Zhao

Re: Address the -Wuse-after-free warning in ATExecAttachPartition()

[Junwang Zhao]

On Tue, Jul 9, 2024 at 6:18 PM Amit Langote  wrote:
>
> Hi Junwang,
>
> On Mon, Jul 8, 2024 at 7:08 PM Junwang Zhao  wrote:
> > On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav
> >  wrote:
> > >
> > > In [1], Andres reported a -Wuse-after-free bug in the
> > > ATExecAttachPartition() function.  I've created a patch to address it
> > > with pointers from Amit offlist.
> > >
> > > The issue was that the partBoundConstraint variable was utilized after
> > > the list_concat() function. This could potentially lead to accessing
> > > the partBoundConstraint variable after its memory has been freed.
> > >
> > > The issue was resolved by using the return value of the list_concat()
> > > function, instead of using the list1 argument of list_concat(). I
> > > copied the partBoundConstraint variable to a new variable named
> > > partConstraint and used it for the previous references before invoking
> > > get_proposed_default_constraint(). I confirmed that the
> > > eval_const_expressions(), make_ands_explicit(),
> > > map_partition_varattnos(), QueuePartitionConstraintValidation()
> > > functions do not modify the memory location pointed to by the
> > > partBoundConstraint variable. Therefore, it is safe to use it for the
> > > next reference in get_proposed_default_constraint()
> > >
> > > Attaching the patch. Please review and share the comments if any.
> > > Thanks to Andres for spotting the bug and some off-list advice on how
> > > to reproduce it.
> >
> > The patch LGTM.
> >
> > Curious how to reproduce the bug ;)
>
> Download and apply (`git am`) Andres' patch to "add allocator
> attributes" here (note it's not merged into the tree yet!):
> https://github.com/anarazel/postgres/commit/99067d5c944e7bd29a8702689f470f623723f4e7.patch
>
> Then configure using meson with -Dc_args="-Wuse-after-free=3"
> --buildtype=release
>
> Compiling with gcc-12 or gcc-13 should give you the warning that looks
> as follows:
>
> [713/2349] Compiling C object
> src/backend/postgres_lib.a.p/commands_tablecmds.c.o
> ../src/backend/commands/tablecmds.c: In function ‘ATExecAttachPartition’:
> ../src/backend/commands/tablecmds.c:18593:25: warning: pointer
> ‘partBoundConstraint’ may be used after ‘list_concat’
> [-Wuse-after-free]
> 18593 |
> get_proposed_default_constraint(partBoundConstraint);
>   |
> ^~~~
> ../src/backend/commands/tablecmds.c:18546:26: note: call to ‘list_concat’ here
> 18546 | partConstraint = list_concat(partBoundConstraint,
>   |  ^~~~
> 18547 |
>   RelationGetPartitionQual(rel));
>   |
>   ~~
>
> --
> Thanks, Amit Langote

Thanks Amit,

Good to know.

When Nitin says:

```Thanks to Andres for spotting the bug and some off-list advice on how
to reproduce it.```

I thought maybe there is some test case that can really trigger the
use-after-free bug, I might get it wrong though ;)


-- 
Regards
Junwang Zhao

Re: jsonpath: Inconsistency of timestamp_tz() Output

[Junwang Zhao]

On Mon, Jul 1, 2024 at 11:02 PM David E. Wheeler  wrote:
>
> Hackers,
>
> There’s an odd difference in the behavior of timestamp_tz() outputs. Running 
> with America/New_York as my TZ, it looks fine for a full timestamptz, 
> identical to how casting the types directly works:
>
> david=# set time zone 'America/New_York';
> SET
>
> david=# select '2024-08-15 12:34:56-04'::timestamptz;
>   timestamptz
> 
>  2024-08-15 12:34:56-04
> (1 row)
>
> david=# select jsonb_path_query_tz('"2024-08-15 12:34:56-04"', 
> '$.timestamp_tz()');
>  jsonb_path_query_tz
> -
>  "2024-08-15T12:34:56-04:00"

# select jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');

Do you also expect this to show the time in America/New_York?

This is what I get:

[local] postgres@postgres:5432-28176=# select
jsonb_path_query_tz('"2024-08-15 12:34:56-05"', '$.timestamp_tz()');
┌─┐
│ jsonb_path_query_tz │
├─┤
│ "2024-08-15T12:34:56-05:00" │
└─┘
(1 row)

The logic in executeDateTimeMethod seems to convert the input to a UTC
timestamp base on the session TZ,
the output seems not cast based on the TZ.

>
> Both show the time in America/New_York, which is great. But when casting from 
> a date, the behavior differs. Casting directly:
>
> david=# select '2024-08-15'::date::timestamptz;
>   timestamptz
> 
>  2024-08-15 00:00:00-04
>
> It stringifies to the current zone setting again, as expected. But look at 
> the output from a path query:
>
> david=# select jsonb_path_query_tz('"2023-08-15"', '$.timestamp_tz()');
>  jsonb_path_query_tz
> -
>  "2023-08-15T04:00:00+00:00"
>
> It’s using UTC for the display output! Shouldn’t it be using America/New_York?
>
> Note that I’m comparing a cast from date to timestamptz because that’s how 
> the jsonpath parsing works[1]: it ultimately uses 
> date2timestamptz_opt_overflow()[2] to make the conversion, which appears to 
> set the offset from the time zone GUC, so I’m not sure where it’s converted 
> to UTC before stringifying.
>
> Maybe an argument is missing from the stringification path?
>
> FWIW, explicitly calling the string() jsonpath method does produce a result 
> in the current TZ:
>
> david=# select jsonb_path_query_tz('"2023-08-15"', 
> '$.timestamp_tz().string()');
>jsonb_path_query_tz
> --
>  "2023-08-15 00:00:00-04"
>
> That bit uses timestamptz_out to format the output, but JSONB has its own 
> stringification[4] (called here[5]), but I can’t tell what might be different 
> between a timestamptz cast from a date and one not cast from a date.
>
> Note the same divergency in behavior occurs when the source value is a 
> timestamp, too. Compare:
>
> david=# select '2024-08-15 12:34:56'::timestamp::timestamptz;
>   timestamptz
> 
>  2024-08-15 12:34:56-04
> (1 row)
>
> david=# select jsonb_path_query_tz('"2023-08-15 12:34:56"', 
> '$.timestamp_tz()');
>  jsonb_path_query_tz
> -
>  "2023-08-15T16:34:56+00:00"
> (1 row)
>
> Anyway, should the output of timestamptz JSONB values be made more 
> consistent? I’m happy to make a patch to do so, but could use a hand figuring 
> out where the behavior varies.
>
> Best,
>
> David
>
> [1]: 
> https://github.com/postgres/postgres/blob/3497c87/src/backend/utils/adt/jsonpath_exec.c#L2708-L2718
> [2]: 
> https://github.com/postgres/postgres/blob/3497c87/src/backend/utils/adt/date.c#L613-L698
> [3]: 
> https://github.com/postgres/postgres/blob/3fb59e789dd9f21610101d1ec106ad58095e24f3/src/backend/utils/adt/jsonpath_exec.c#L1650-L1653
> [4]: 
> https://github.com/postgres/postgres/blob/3fb59e789dd9f21610101d1ec106ad58095e24f3/src/backend/utils/adt/json.c#L369-L407
> [5]: 
> https://github.com/postgres/postgres/blob/3fb59e7/src/backend/utils/adt/jsonb.c#L743-L748
>
>
>


-- 
Regards
Junwang Zhao

Re: Address the -Wuse-after-free warning in ATExecAttachPartition()

[Junwang Zhao]

On Mon, Jul 8, 2024 at 3:22 PM Nitin Jadhav
 wrote:
>
> In [1], Andres reported a -Wuse-after-free bug in the
> ATExecAttachPartition() function.  I've created a patch to address it
> with pointers from Amit offlist.
>
> The issue was that the partBoundConstraint variable was utilized after
> the list_concat() function. This could potentially lead to accessing
> the partBoundConstraint variable after its memory has been freed.
>
> The issue was resolved by using the return value of the list_concat()
> function, instead of using the list1 argument of list_concat(). I
> copied the partBoundConstraint variable to a new variable named
> partConstraint and used it for the previous references before invoking
> get_proposed_default_constraint(). I confirmed that the
> eval_const_expressions(), make_ands_explicit(),
> map_partition_varattnos(), QueuePartitionConstraintValidation()
> functions do not modify the memory location pointed to by the
> partBoundConstraint variable. Therefore, it is safe to use it for the
> next reference in get_proposed_default_constraint()
>
> Attaching the patch. Please review and share the comments if any.
> Thanks to Andres for spotting the bug and some off-list advice on how
> to reproduce it.

The patch LGTM.

Curious how to reproduce the bug ;)

>
> [1]: 
> https://www.postgresql.org/message-id/flat/202311151802.ngj2la66jwgi%40alvherre.pgsql#4fc5622772ba0244c1ad203f5fc56701
>
> Best Regards,
> Nitin Jadhav
> Azure Database for PostgreSQL
> Microsoft



-- 
Regards
Junwang Zhao

a potential typo in comments of pg_parse_json

[Junwang Zhao]

Not 100% sure, sorry if this doesn't make sense.

--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -514,7 +514,7 @@ freeJsonLexContext(JsonLexContext *lex)
  *
  * If FORCE_JSON_PSTACK is defined then the routine will call the non-recursive
  * JSON parser. This is a useful way to validate that it's doing the right
- * think at least for non-incremental cases. If this is on we expect to see
+ * thing at least for non-incremental cases. If this is on we expect to see
  * regression diffs relating to error messages about stack depth, but no
  * other differences.
  */


-- 
Regards
Junwang Zhao

report a typo in comments of ComputeXidHorizonsResult

[Junwang Zhao]

I think the period here should be a typo.

index 16b5803d388..af3b15e93df 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -185,7 +185,7 @@ typedef struct ComputeXidHorizonsResult
FullTransactionId latest_completed;

/*
-* The same for procArray->replication_slot_xmin and.
+* The same for procArray->replication_slot_xmin and
 * procArray->replication_slot_catalog_xmin.
 */

-- 
Regards
Junwang Zhao

Re: Extension using Meson as build system

[Junwang Zhao]

On Sun, Jun 30, 2024 at 9:31 PM Pavel Stehule  wrote:
>
>
>
> ne 30. 6. 2024 v 15:28 odesílatel Junwang Zhao  napsal:
>>
>> On Sun, Jun 30, 2024 at 9:20 PM Pavel Stehule  
>> wrote:
>> >
>> > Hi
>> >
>> > ne 30. 6. 2024 v 15:17 odesílatel Junwang Zhao  napsal:
>> >>
>> >> Hi  hackers,
>> >>
>> >> Is there any extension that uses meson as build systems?
>> >> I'm starting a extension project that written in c++, cmake is my
>> >> initial choice as the build system, but since PostgreSQL has adopted
>> >> Meson, so I'm wondering if there is any extension that also uses
>> >> meson that I can reference.
>> >
>> >
>> > any extension from contrib package
>>
>> Ah, yeah, but I'm not sure these extensions have any dependencies
>> on the main project? Never mind, I'll take a look, thanks.
>
>
> any postgres extension has dependency on main project
>
> what can be different - if the extension is build inside or outside source 
> code tree

Take contrib/ltree as an example, in Makefile, there are some lines:

ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
include $(PGXS)
else
subdir = contrib/ltree
top_builddir = ../..
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif

I am taking these as the reason that extension can build outside of postgres
source code, but I can't find an equivalent in meson.build.


>
>>
>>
>> >
>> > https://github.com/postgres/postgres/tree/master/contrib
>> >
>> > probably only these
>> >
>> > Regards
>> >
>> > Pavel
>> >>
>> >>
>> >> --
>> >> Regards
>> >> Junwang Zhao
>> >>
>> >>
>>
>>
>> --
>> Regards
>> Junwang Zhao



-- 
Regards
Junwang Zhao

Re: Extension using Meson as build system

[Junwang Zhao]

On Sun, Jun 30, 2024 at 9:20 PM Pavel Stehule  wrote:
>
> Hi
>
> ne 30. 6. 2024 v 15:17 odesílatel Junwang Zhao  napsal:
>>
>> Hi  hackers,
>>
>> Is there any extension that uses meson as build systems?
>> I'm starting a extension project that written in c++, cmake is my
>> initial choice as the build system, but since PostgreSQL has adopted
>> Meson, so I'm wondering if there is any extension that also uses
>> meson that I can reference.
>
>
> any extension from contrib package

Ah, yeah, but I'm not sure these extensions have any dependencies
on the main project? Never mind, I'll take a look, thanks.

>
> https://github.com/postgres/postgres/tree/master/contrib
>
> probably only these
>
> Regards
>
> Pavel
>>
>>
>> --
>> Regards
>> Junwang Zhao
>>
>>


-- 
Regards
Junwang Zhao

Extension using Meson as build system

[Junwang Zhao]

Hi  hackers,

Is there any extension that uses meson as build systems?
I'm starting a extension project that written in c++, cmake is my
initial choice as the build system, but since PostgreSQL has adopted
Meson, so I'm wondering if there is any extension that also uses
meson that I can reference.

-- 
Regards
Junwang Zhao

stale comments about fastgetattr and heap_getattr

[Junwang Zhao]

fastgetattr and heap_getattr are converted to inline functions
in e27f4ee0a701, while some comments still referring them as macros.

-- 
Regards
Junwang Zhao


0001-stale-comments-about-fastgetattr-and-heap_getattr.patch
Description: Binary data

Re: Improve conditional compilation for direct I/O alignment checks

[Junwang Zhao]

On Sun, May 26, 2024 at 3:16 PM Junwang Zhao  wrote:
>
> This patch refactors the alignment checks for direct I/O to preprocess phase,
> thereby reducing some CPU cycles.
>
> --
> Regards
> Junwang Zhao

Patch v2 with some additional minor polishment of the comments in `mdwriteback`.

-- 
Regards
Junwang Zhao


v2-0001-Improve-conditional-compilation-for-direct-I-O-al.patch
Description: Binary data

Improve conditional compilation for direct I/O alignment checks

[Junwang Zhao]

This patch refactors the alignment checks for direct I/O to preprocess phase,
thereby reducing some CPU cycles.

-- 
Regards
Junwang Zhao


0001-Improve-conditional-compilation-for-direct-I-O-align.patch
Description: Binary data

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Fri, Feb 2, 2024 at 2:21 PM Michael Paquier  wrote:
>
> On Fri, Feb 02, 2024 at 09:40:56AM +0900, Sutou Kouhei wrote:
> > Thanks. It'll help us.
>
> I have done a review of v10, see v11 attached which is still WIP, with
> the patches for COPY TO and COPY FROM merged together.  Note that I'm
> thinking to merge them into a single commit.
>
> @@ -74,11 +75,11 @@ typedef struct CopyFormatOptions
>  boolconvert_selectively;/* do selective binary conversion? */
>  CopyOnErrorChoice on_error; /* what to do when error happened */
>  List   *convert_select; /* list of column names (can be NIL) */
> +constCopyToRoutine *to_routine;/* callback routines for COPY 
> TO */
>  } CopyFormatOptions;
>
> Adding the routines to the structure for the format options is in my
> opinion incorrect.  The elements of this structure are first processed
> in the option deparsing path, and then we need to use the options to
> guess which routines we need.  A more natural location is cstate
> itself, so as the pointer to the routines is isolated within copyto.c

I agree CopyToRoutine should be placed into CopyToStateData, but
why set it after ProcessCopyOptions, the implementation of
CopyToGetRoutine doesn't make sense if we want to support custom
format in the future.

Seems the refactor of v11 only considered performance but not
*extendable copy format*.

> and copyfrom_internal.h.  My point is: the routines are an
> implementation detail that the centralized copy.c has no need to know
> about.  This also led to a strange separation with
> ProcessCopyOptionFormatFrom() and ProcessCopyOptionFormatTo() to fit
> the hole in-between.
>
> The separation between cstate and the format-related fields could be
> much better, though I am not sure if it is worth doing as it
> introduces more duplication.  For example, max_fields and raw_fields
> are specific to text and csv, while binary does not care much.
> Perhaps this is just useful to be for custom formats.

I think those can be placed in format specific fields by utilizing the opaque
space, but yeah, this will introduce duplication.

>
> copyapi.h needs more documentation, like what is expected for
> extension developers when using these, what are the arguments, etc.  I
> have added what I had in mind for now.
>
> +typedef char *(*PostpareColumnValue) (CopyFromState cstate, char *string, 
> int m);
>
> CopyReadAttributes and PostpareColumnValue are also callbacks specific
> to text and csv, except that they are used within the per-row
> callbacks.  The same can be said about CopyAttributeOutHeaderFunction.
> It seems to me that it would be less confusing to store pointers to
> them in the routine structures, where the final picture involves not
> having multiple layers of APIs like CopyToCSVStart,
> CopyAttributeOutTextValue, etc.  These *have* to be documented
> properly in copyapi.h, and this is much easier now that cstate stores
> the routine pointers.  That would also make simpler function stacks.
> Note that I have not changed that in the v11 attached.
>
> This business with the extra callbacks required for csv and text is my
> main point of contention, but I'd be OK once the model of the APIs is
> more linear, with everything in Copy{From,To}State.  The changes would
> be rather simple, and I'd be OK to put my hands on it.  Just,
> Sutou-san, would you agree with my last point about these extra
> callbacks?
> --
> Michael

If V7 and V10 have no performance reduction, then I think V6 is also
good with performance, since most of the time goes to CopyToOneRow
and CopyFromOneRow.

I just think we should take the *extendable* into consideration at
the beginning.

-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Thu, Feb 1, 2024 at 11:56 AM Michael Paquier  wrote:
>
> On Thu, Feb 01, 2024 at 11:43:07AM +0800, Junwang Zhao wrote:
> > The first 6 rounds are like 10% better than the later 9 rounds, is this 
> > normal?
>
> Even with HEAD?  Perhaps you have some OS cache eviction in play here?
> FWIW, I'm not seeing any of that with longer runs after 7~ tries in a
> loop of 15.

Yeah, with HEAD. I'm on ubuntu 22.04, I did not change any gucs, maybe I should
set a higher shared_buffers? But I dought that's related ;(


> --
> Michael



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

Hi Michael,

On Thu, Feb 1, 2024 at 9:58 AM Michael Paquier  wrote:
>
> On Wed, Jan 31, 2024 at 02:39:54PM +0900, Michael Paquier wrote:
> > Thanks, I'm looking into that now.
>
> I have much to say about the patch, but for now I have begun running
> some performance tests using the patches, because this thread won't
> get far until we are sure that the callbacks do not impact performance
> in some kind of worst-case scenario.  First, here is what I used to
> setup a set of tables used for COPY FROM and COPY TO (requires [1] to
> feed COPY FROM's data to the void, and note that default values is to
> have a strict control on the size of the StringInfos used in the copy
> paths):
> CREATE EXTENSION blackhole_am;
> CREATE OR REPLACE FUNCTION create_table_cols(tabname text, num_cols int)
> RETURNS VOID AS
> $func$
> DECLARE
>   query text;
> BEGIN
>   query := 'CREATE UNLOGGED TABLE ' || tabname || ' (';
>   FOR i IN 1..num_cols LOOP
> query := query || 'a_' || i::text || ' int default 1';
> IF i != num_cols THEN
>   query := query || ', ';
> END IF;
>   END LOOP;
>   query := query || ')';
>   EXECUTE format(query);
> END
> $func$ LANGUAGE plpgsql;
> -- Tables used for COPY TO
> SELECT create_table_cols ('to_tab_1', 1);
> SELECT create_table_cols ('to_tab_10', 10);
> INSERT INTO to_tab_1 SELECT FROM generate_series(1, 1000);
> INSERT INTO to_tab_10 SELECT FROM generate_series(1, 1000);
> -- Data for COPY FROM
> COPY to_tab_1 TO '/tmp/to_tab_1.bin' WITH (format binary);
> COPY to_tab_10 TO '/tmp/to_tab_10.bin' WITH (format binary);
> COPY to_tab_1 TO '/tmp/to_tab_1.txt' WITH (format text);
> COPY to_tab_10 TO '/tmp/to_tab_10.txt' WITH (format text);
> -- Tables used for COPY FROM
> SELECT create_table_cols ('from_tab_1', 1);
> SELECT create_table_cols ('from_tab_10', 10);
> ALTER TABLE from_tab_1 SET ACCESS METHOD blackhole_am;
> ALTER TABLE from_tab_10 SET ACCESS METHOD blackhole_am;
>
> Then I have run a set of tests using HEAD, v7 and v10 with queries
> like that (adapt them depending on the format and table):
> COPY to_tab_1 TO '/dev/null' WITH (FORMAT text) \watch count=5
> SET client_min_messages TO error; -- for blackhole_am
> COPY from_tab_1 FROM '/tmp/to_tab_1.txt' with (FORMAT 'text') \watch count=5
> COPY from_tab_1 FROM '/tmp/to_tab_1.bin' with (FORMAT 'binary') \watch count=5
>
> All the patches have been compiled with -O2, without assertions, etc.
> Postgres is run in tmpfs mode, on scissors, without fsync.  Unlogged
> tables help a bit in focusing on the execution paths as we don't care
> about WAL, of course.  I have also included v7 in the test of tests,
> as this version uses more simple per-row callbacks.
>
> And here are the results I get for text and binary (ms, average of 15
> queries after discarding the three highest and three lowest values):
>   test   | master |  v7  | v10
> -++--+--
>  from_bin_1col   | 1575   | 1546 | 1575
>  from_bin_10col  | 5364   | 5208 | 5230
>  from_text_1col  | 1690   | 1715 | 1684
>  from_text_10col | 4875   | 4793 | 4757
>  to_bin_1col | 1717   | 1730 | 1731
>  to_bin_10col| 7728   | 7707 | 7513
>  to_text_1col| 1710   | 1730 | 1698
>  to_text_10col   | 5998   | 5960 | 5987
>
> I am getting an interesting trend here in terms of a speedup between
> HEAD and the patches with a table that has 10 attributes filled with
> integers, especially for binary and text with COPY FROM.  COPY TO
> binary also gets nice numbers, while text looks rather stable.  Hmm.
>
> These were on my buildfarm animal, but we need to be more confident
> about all this.  Could more people run these tests?  I am going to do
> a second session on a local machine I have at hand and see what
> happens.  Will publish the numbers here, the method will be the same.
>
> [1]: https://github.com/michaelpq/pg_plugins/tree/main/blackhole_am
> --
> Michael

I'm running the benchmark, but I got some strong numbers:

postgres=# \timing
Timing is on.
postgres=# COPY to_tab_10 TO '/dev/null' WITH (FORMAT binary) \watch count=15
COPY 1000
Time: 3168.497 ms (00:03.168)
COPY 1000
Time: 3255.464 ms (00:03.255)
COPY 1000
Time: 3270.625 ms (00:03.271)
COPY 1000
Time: 3285.112 ms (00:03.285)
COPY 1000
Time: 3322.304 ms (00:03.322)
COPY 1000
Time: 3341.328 ms (00:03.341)
COPY 1000
Time: 3621.564 ms (00:03.622)
COPY 1000
Time: 3700.911 ms (00:03.701)
COPY 1000
Time: 3717.992 ms (00:03.718)
COPY 1000
Time: 3708.350 ms (00:03.708)
COPY 1000
Time: 3704.367 ms (00:03.704)
COPY 1000
Time: 3724.281 ms (00:03.724)
COPY 1000
Time: 3703.335 ms (00:03.703)
COPY 1000
Time: 3728.629 ms (00:03.729)
COPY 1000
Time: 3758.135 ms (00:03.758)

The first 6 rounds are like 10% better than the later 9 rounds, is this normal?

-- 
Regards
Junwang Zhao

Re: Emitting JSON to file using COPY TO

[Junwang Zhao]

Hi Vignesh,

On Wed, Jan 31, 2024 at 5:50 PM vignesh C  wrote:
>
> On Sat, 27 Jan 2024 at 11:25, Junwang Zhao  wrote:
> >
> > Hi hackers,
> >
> > Kou-san(CCed) has been working on *Make COPY format extendable[1]*, so
> > I think making *copy to json* based on that work might be the right 
> > direction.
> >
> > I write an extension for that purpose, and here is the patch set together
> > with Kou-san's *extendable copy format* implementation:
> >
> > 0001-0009 is the implementation of extendable copy format
> > 00010 is the pg_copy_json extension
> >
> > I also created a PR[2] if anybody likes the github review style.
> >
> > The *extendable copy format* feature is still being developed, I post this
> > email in case the patch set in this thread is committed without knowing
> > the *extendable copy format* feature.
> >
> > I'd like to hear your opinions.
>
> CFBot shows that one of the test is failing as in [1]:
> [05:46:41.678] /bin/sh: 1: cannot open
> /tmp/cirrus-ci-build/contrib/pg_copy_json/sql/test_copy_format.sql: No
> such file
> [05:46:41.678] diff:
> /tmp/cirrus-ci-build/contrib/pg_copy_json/expected/test_copy_format.out:
> No such file or directory
> [05:46:41.678] diff:
> /tmp/cirrus-ci-build/contrib/pg_copy_json/results/test_copy_format.out:
> No such file or directory
> [05:46:41.678] # diff command failed with status 512: diff
> "/tmp/cirrus-ci-build/contrib/pg_copy_json/expected/test_copy_format.out"
> "/tmp/cirrus-ci-build/contrib/pg_copy_json/results/test_copy_format.out"
> > "/tmp/cirrus-ci-build/contrib/pg_copy_json/results/test_copy_format.out.diff"
> [05:46:41.678] Bail out!make[2]: *** [../../src/makefiles/pgxs.mk:454:
> check] Error 2
> [05:46:41.679] make[1]: *** [Makefile:96: check-pg_copy_json-recurse] Error 2
> [05:46:41.679] make: *** [GNUmakefile:71: check-world-contrib-recurse] Error 2
>
> Please post an updated version for the same.

Thanks for the reminder, the patch set I posted is not for commit but
for further discussion.

I will post more information about the *extendable copy* feature
when it's about to be committed.

>
> [1] - https://cirrus-ci.com/task/5322439115145216
>
> Regards,
> Vignesh



-- 
Regards
Junwang Zhao

Re: UUID v7

[Junwang Zhao]

Hi Andrey,

On Tue, Jan 30, 2024 at 5:56 PM Andrey M. Borodin  wrote:
>
>
>
> > On 30 Jan 2024, at 12:28, Sergey Prokhorenko 
> >  wrote:
> >
> >
> > I think this phrase is outdated: "This function can optionally accept a 
> > timestamp used instead of current time.
> > This allows implementation of k-way sotable identifiers.”
> Fixed.
>
> > This phrase is wrong: "Both functions return a version 4 (random) UUID.”
> This applies to functions gen_random_uuid() and uuidv4().
> >
> > For this phrase the reason is unclear and the phrase is most likely 
> > incorrect:
> > if large batches of UUIDs are generated at the
> > +   same time it's possible that some UUIDs will store a time that is 
> > slightly later
> > +   than their actual generation time
>
> I’ve rewritten this phrase, hope it’s more clear now.
>
>
> Best regards, Andrey Borodin.

+Datum
+uuid_extract_var(PG_FUNCTION_ARGS)
+{
+ pg_uuid_t  *uuid = PG_GETARG_UUID_P(0);
+ uint16_t result;
+ result = uuid->data[8] >> 6;
+
+ PG_RETURN_UINT16(result);
+}
\ No newline at end of file

It's always good to add a newline at the end of a  source file, though
this might be nitpicky.

-- 
Regards
Junwang Zhao

Re: UUID v7

[Junwang Zhao]

e the clock value needs to
> > be to the actual time.
>
> I see no reason why we cannot make stronger guarantees about the
> timestamps that we use to generate UUIDs with our uuidv7() function.
> And then we can update the documentation for
> uuid_extract_time to something like this:
>
> > This function extracts a timestamptz from UUID versions 1, 6 and 7. For 
> > other
> > versions and variants this function returns NULL. The extracted timestamp
> > does not necessarily equate to the time of UUID generation. How close it is
> > to the actual time depends on the implementation that generated to UUID.
> > The uuidv7() function provided PostgreSQL will normally store the actual 
> > time of
> > generation to in the UUID, but if large batches of UUIDs are generated at 
> > the
> > same time it's possible that some UUIDs will store a time that is slightly 
> > later
> > than their actual generation time.
>
>


-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Mon, Jan 29, 2024 at 2:03 PM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Sat, 27 Jan 2024 14:15:02 +0800,
>   Junwang Zhao  wrote:
>
> > I have been working on a *COPY TO JSON* extension since yesterday,
> > which is based on your V6 patch set, I'd like to give you more input
> > so you can make better decisions about the implementation(with only
> > pg-copy-arrow you might not get everything considered).
>
> Thanks!
>
> > 0009 is some changes made by me, I changed CopyToGetFormat to
> > CopyToSendCopyBegin because pg_copy_json need to send different bytes
> > in SendCopyBegin, get the format code along is not enough
>
> Oh, I haven't cared about the case.
> How about the following API instead?
>
> static void
> SendCopyBegin(CopyToState cstate)
> {
> StringInfoData buf;
>
> pq_beginmessage(, PqMsg_CopyOutResponse);
> cstate->opts.to_routine->CopyToFillCopyOutResponse(cstate, );
> pq_endmessage();
> cstate->copy_dest = COPY_FRONTEND;
> }
>
> static void
> CopyToJsonFillCopyOutResponse(CopyToState cstate, StringInfoData )
> {
> int16   format = 0;
>
> pq_sendbyte(, format);  /* overall format */
> /*
>  * JSON mode is always one non-binary column
>  */
> pq_sendint16(, 1);
> pq_sendint16(, format);
> }

Make sense to me.

>
> > 00010 is the pg_copy_json extension, I think this should be a good
> > case which can utilize the *extendable copy format* feature
>
> It seems that it's convenient that we have one more callback
> for initializing CopyToState::opaque. It's called only once
> when Copy{To,From}Routine is chosen:
>
> typedef struct CopyToRoutine
> {
> void(*CopyToInit) (CopyToState cstate);
> ...
> };

I like this, we can alloc private data in this hook.

>
> void
> ProcessCopyOptions(ParseState *pstate,
>CopyFormatOptions *opts_out,
>bool is_from,
>void *cstate,
>List *options)
> {
> ...
> foreach(option, options)
> {
> DefElem*defel = lfirst_node(DefElem, option);
>
> if (strcmp(defel->defname, "format") == 0)
> {
> ...
> opts_out->to_routine = 
> opts_out->to_routine->CopyToInit(cstate);
> ...
> }
> }
> ...
> }
>
>
> >  maybe we
> > should delete copy_test_format if we have this extension as an
> > example?
>
> I haven't read the COPY TO format json thread[1] carefully
> (sorry), but we may add the JSON format as a built-in
> format. If we do it, copy_test_format is useful to test the
> extension API.

Yeah, maybe, I have no strong opinion here, pg_copy_json is
just a toy extension for discussion.

>
> [1] 
> https://www.postgresql.org/message-id/flat/CALvfUkBxTYy5uWPFVwpk_7ii2zgT07t3d-yR_cy4sfrrLU%3Dkcg%40mail.gmail.com
>
>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Mon, Jan 29, 2024 at 11:22 AM Masahiko Sawada  wrote:
>
> On Mon, Jan 29, 2024 at 12:10 PM Junwang Zhao  wrote:
> >
> > On Mon, Jan 29, 2024 at 10:42 AM Masahiko Sawada  
> > wrote:
> > >
> > > On Fri, Jan 26, 2024 at 6:02 PM Junwang Zhao  wrote:
> > > >
> > > > On Fri, Jan 26, 2024 at 4:55 PM Sutou Kouhei  
> > > > wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > In 
> > > > > 
> > > > >   "Re: Make COPY format extendable: Extract COPY TO format 
> > > > > implementations" on Fri, 26 Jan 2024 16:41:50 +0800,
> > > > >   Junwang Zhao  wrote:
> > > > >
> > > > > > CopyToProcessOption()/CopyFromProcessOption() can only handle
> > > > > > single option, and store the options in the opaque field,  but it 
> > > > > > can not
> > > > > > check the relation of two options, for example, considering json 
> > > > > > format,
> > > > > > the `header` option can not be handled by these two functions.
> > > > > >
> > > > > > I want to find a way when the user specifies the header option, 
> > > > > > customer
> > > > > > handler can error out.
> > > > >
> > > > > Ah, you want to use a built-in option (such as "header")
> > > > > value from a custom handler, right? Hmm, it may be better
> > > > > that we call CopyToProcessOption()/CopyFromProcessOption()
> > > > > for all options including built-in options.
> > > > >
> > > > Hmm, still I don't think it can handle all cases, since we don't know
> > > > the sequence of the options, we need all the options been parsed
> > > > before we check the compatibility of the options, or customer
> > > > handlers will need complicated logic to resolve that, which might
> > > > lead to ugly code :(
> > > >
> > >
> > > Does it make sense to pass only non-builtin options to the custom
> > > format callback after parsing and evaluating the builtin options? That
> > > is, we parse and evaluate only the builtin options and populate
> > > opts_out first, then pass each rest option to the custom format
> > > handler callback. The callback can refer to the builtin option values.
> >
> > Yeah, I think this makes sense.
> >
> > > The callback is expected to return false if the passed option is not
> > > supported. If one of the builtin formats is specified and the rest
> > > options list has at least one option, we raise "option %s not
> > > recognized" error.  IOW it's the core's responsibility to ranse the
> > > "option %s not recognized" error, which is in order to raise a
> > > consistent error message. Also, I think the core should check the
> > > redundant options including bultiin and custom options.
> >
> > It would be good that core could check all the redundant options,
> > but where should core do the book-keeping of all the options? I have
> > no idea about this, in my implementation of pg_copy_json extension,
> > I handle redundant options by adding a xxx_specified field for each
> > xxx.
>
> What I imagined is that while parsing the all specified options, we
> evaluate builtin options and we add non-builtin options to another
> list. Then when parsing a non-builtin option, we check if this option
> already exists in the list. If there is, we raise the "option %s not
> recognized" error.". Once we complete checking all options, we pass
> each option in the list to the callback.

LGTM.

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



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Mon, Jan 29, 2024 at 10:42 AM Masahiko Sawada  wrote:
>
> On Fri, Jan 26, 2024 at 6:02 PM Junwang Zhao  wrote:
> >
> > On Fri, Jan 26, 2024 at 4:55 PM Sutou Kouhei  wrote:
> > >
> > > Hi,
> > >
> > > In 
> > >   "Re: Make COPY format extendable: Extract COPY TO format 
> > > implementations" on Fri, 26 Jan 2024 16:41:50 +0800,
> > >   Junwang Zhao  wrote:
> > >
> > > > CopyToProcessOption()/CopyFromProcessOption() can only handle
> > > > single option, and store the options in the opaque field,  but it can 
> > > > not
> > > > check the relation of two options, for example, considering json format,
> > > > the `header` option can not be handled by these two functions.
> > > >
> > > > I want to find a way when the user specifies the header option, customer
> > > > handler can error out.
> > >
> > > Ah, you want to use a built-in option (such as "header")
> > > value from a custom handler, right? Hmm, it may be better
> > > that we call CopyToProcessOption()/CopyFromProcessOption()
> > > for all options including built-in options.
> > >
> > Hmm, still I don't think it can handle all cases, since we don't know
> > the sequence of the options, we need all the options been parsed
> > before we check the compatibility of the options, or customer
> > handlers will need complicated logic to resolve that, which might
> > lead to ugly code :(
> >
>
> Does it make sense to pass only non-builtin options to the custom
> format callback after parsing and evaluating the builtin options? That
> is, we parse and evaluate only the builtin options and populate
> opts_out first, then pass each rest option to the custom format
> handler callback. The callback can refer to the builtin option values.

Yeah, I think this makes sense.

> The callback is expected to return false if the passed option is not
> supported. If one of the builtin formats is specified and the rest
> options list has at least one option, we raise "option %s not
> recognized" error.  IOW it's the core's responsibility to ranse the
> "option %s not recognized" error, which is in order to raise a
> consistent error message. Also, I think the core should check the
> redundant options including bultiin and custom options.

It would be good that core could check all the redundant options,
but where should core do the book-keeping of all the options? I have
no idea about this, in my implementation of pg_copy_json extension,
I handle redundant options by adding a xxx_specified field for each
xxx.

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



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Fri, Jan 26, 2024 at 4:55 PM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Fri, 26 Jan 2024 16:41:50 +0800,
>   Junwang Zhao  wrote:
>
> > CopyToProcessOption()/CopyFromProcessOption() can only handle
> > single option, and store the options in the opaque field,  but it can not
> > check the relation of two options, for example, considering json format,
> > the `header` option can not be handled by these two functions.
> >
> > I want to find a way when the user specifies the header option, customer
> > handler can error out.
>
> Ah, you want to use a built-in option (such as "header")
> value from a custom handler, right? Hmm, it may be better
> that we call CopyToProcessOption()/CopyFromProcessOption()
> for all options including built-in options.
>
Hmm, still I don't think it can handle all cases, since we don't know
the sequence of the options, we need all the options been parsed
before we check the compatibility of the options, or customer
handlers will need complicated logic to resolve that, which might
lead to ugly code :(

>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Fri, Jan 26, 2024 at 4:32 PM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Fri, 26 Jan 2024 16:18:14 +0800,
>   Junwang Zhao  wrote:
>
> > In the current implementation, there is no way that one can check
> > incompatibility
> > options in ProcessCopyOptions, we can postpone the check in CopyFromStart
> > or CopyToStart, but I think it is a little bit late. Do you think
> > adding an extra
> > check for incompatible options hook is acceptable (PFA)?
>
> Thanks for the suggestion! But I think that a custom handler
> can do it in
> CopyToProcessOption()/CopyFromProcessOption(). What do you
> think about this? Or could you share a sample COPY TO/FROM
> WITH() SQL you think?

CopyToProcessOption()/CopyFromProcessOption() can only handle
single option, and store the options in the opaque field,  but it can not
check the relation of two options, for example, considering json format,
the `header` option can not be handled by these two functions.

I want to find a way when the user specifies the header option, customer
handler can error out.

>
>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Thu, Jan 25, 2024 at 4:52 PM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Thu, 25 Jan 2024 13:36:03 +0900,
>   Masahiko Sawada  wrote:
>
> > I've experimented with a similar optimization for csv
> > and text format; have different callbacks for text and csv format and
> > remove "if (cstate->opts.csv_mode)" branches. I've attached a patch
> > for that. Here are results:
> >
> > HEAD w/ 0001 patch + remove branches:
> > binary 2824.502 ms
> > text 2715.264 ms
> > csv 2803.381 ms
> >
> > The numbers look better now. I'm not sure these are within a noise
> > range but it might be worth considering having different callbacks for
> > text and csv formats.
>
> Wow! Interesting. I tried the approach before but I didn't
> see any difference by the approach. But it may depend on my
> environment.
>
> I'll import the approach to the next patch set so that
> others can try the approach easily.
>
>
> Thanks,
> --
> kou

Hi Kou-san,

In the current implementation, there is no way that one can check
incompatibility
options in ProcessCopyOptions, we can postpone the check in CopyFromStart
or CopyToStart, but I think it is a little bit late. Do you think
adding an extra
check for incompatible options hook is acceptable (PFA)?


-- 
Regards
Junwang Zhao


0001-add-check-incomptiblity-options-hooks.patch
Description: Binary data

Re: make dist using git archive

[Junwang Zhao]

On Tue, Jan 23, 2024 at 2:36 AM Peter Eisentraut  wrote:
>
> On 22.01.24 13:10, Junwang Zhao wrote:
> > I played this with meson build on macOS, the packages are generated
> > in source root but not build root, I'm sure if this is by design but I think
> > polluting *working directory* is not good.
>
> Yes, it's not good, but I couldn't find a way to make it work.
>
> This is part of the complications with meson I referred to.  The
> @BUILD_ROOT@ placeholder in custom_target() is apparently always a
> relative path, but it doesn't know that git -C changes the current
> directory.
>
> > Another thing I'd like to point out is, should we also introduce *git 
> > commit*
> > or maybe *git tag* to package name, something like:
> >
> > git archive --format tar.gz --prefix postgresql-17devel/ HEAD -o
> > postgresql-17devel-`git rev-parse --short HEAD`.tar.gz
> > git archive --format tar.gz --prefix postgresql-17devel/ HEAD -o
> > postgresql-`git describe --tags`.tar.gz
>
> I'm not sure why we would need it built-in.  It can be done by hand, of
> course.

If this is only used by the release phase, one can do this by hand.

*commit id/tag* in package name can be used to identify the git source,
which might be useful for cooperation between QA and dev team,
but surely there are better ways for this, so I do not have a strong
opinion here.

>


-- 
Regards
Junwang Zhao

Re: make dist using git archive

[Junwang Zhao]

Hi,

On Mon, Jan 22, 2024 at 3:32 PM Peter Eisentraut  wrote:
>
> One of the goals is to make the creation of the distribution tarball
> more directly traceable to the git repository.  That is why we removed
> the "make distprep" step.
>
> Here I want to take another step in that direction, by changing "make
> dist" to directly use "git archive", rather than the custom shell script
> it currently runs.
>
> The simple summary is that it would run
>
> git archive --format tar.gz --prefix postgresql-17devel/ HEAD -o
> postgresql-17devel.tar.gz
>
> (with appropriate version numbers, of course), and that's the tarball we
> would ship.
>
> There are analogous commands for other compression formats.
>
> The actual command gets subtly more complicated if you need to run this
> in a separate build directory.  In my attached patch, the make version
> doesn't support vpath at the moment, just so that it's easier to
> understand for now.  The meson version is a bit hairy.
>
> I have studied and tested this quite a bit, and I have found that the
> archives produced this way are deterministic and reproducible, meaning
> for a given commit the result file should always be bit-for-bit identical.
>
> The exception is that if you use a git version older than 2.38.0, gzip
> records the platform in the archive, so you'd get a different output on
> Windows vs. macOS vs. "UNIX" (everything else).  In git 2.38.0, this was
> changed so that everything is recorded as "UNIX" now.  This is just
> something to keep in mind.  This issue is specific to the gzip format,
> it does not affect other compression formats.
>
> Meson has its own distribution building command (meson dist), but opted
> against using that.  The main problem is that the way they have
> implemented it, it is not deterministic in the above sense.  (Another
> point is of course that we probably want a "make" version for the time
> being.)
>
> But the target name "dist" in meson is reserved for that reason, so I
> needed to call the custom target "pgdist".
>
> I did take one idea from meson: It runs a check before git archive that
> the checkout is clean.  That way, you avoid mistakes because of
> uncommitted changes.  This works well in my "make" implementation.  In
> the meson implementation, I had to find a workaround, because a
> custom_target cannot have a dependency on a run_target.  As also
> mentioned above, the whole meson implementation is a bit ugly.
>
> Anyway,  with the attached patch you can do
>
>  make dist
>
> or
>
>  meson compile -C build pgdist

I played this with meson build on macOS, the packages are generated
in source root but not build root, I'm sure if this is by design but I think
polluting *working directory* is not good.

Another thing I'd like to point out is, should we also introduce *git commit*
or maybe *git tag* to package name, something like:

git archive --format tar.gz --prefix postgresql-17devel/ HEAD -o
postgresql-17devel-`git rev-parse --short HEAD`.tar.gz
git archive --format tar.gz --prefix postgresql-17devel/ HEAD -o
postgresql-`git describe --tags`.tar.gz

>
> and it produces the same set of tarballs as before, except it's done
> differently.
>
> The actual build scripts need some fine-tuning, but the general idea is
> correct, I think.

I think this is a good idea, thanks for working on this.


-- 
Regards
Junwang Zhao

Re: make pg_ctl more friendly

[Junwang Zhao]

Hi Alvaro,

On Wed, Jan 17, 2024 at 4:54 PM Alvaro Herrera  wrote:
>
> I think this needs more comments.  First, in the WaitPMResult enum, we
> currently have three values -- READY, STILL_STARTING, FAILED.  These are
> all pretty self-explanatory.  But this patch adds SHUTDOWN_IN_RECOVERY,
> and that's not at all self-explanatory.  Did postmaster start or not?
> The enum value's name doesn't make that clear.  So I'd do something like
>
> typedef enum
> {
> POSTMASTER_READY,
> POSTMASTER_STILL_STARTING,
> /*
>  * postmaster no longer running, because it stopped after recovery
>  * completed.
>  */
> POSTMASTER_SHUTDOWN_IN_RECOVERY,
> POSTMASTER_FAILED,
> } WaitPMResult;
>
> Maybe put the comments in wait_for_postmaster_start instead.

I put the comments in WaitPMResult since we need to add two
of those if in wait_for_postmaster_start.

>
> Secondly, the patch proposes to add new text to be returned by
> do_start() when this happens, which would look like this:
>
>   waiting for server to start...  shut down while in recovery
>   update recovery target settings for startup again if needed
>
> Is this crystal clear?  I'm not sure.  How about something like this?
>
>   waiting for server to start...  done, but not running
>   server shut down because of recovery target settings

Agreed.
>
> variations on the first phrase:
>
> "done, no longer running"
> "done, but no longer running"
> "done, automatically shut down"
> "done, automatically shut down after recovery"

I chose the last one because it gives information to users.
See V5, thanks for the wording ;)

>
> --
> Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
> "Now I have my system running, not a byte was off the shelf;
> It rarely breaks and when it does I fix the code myself.
> It's stable, clean and elegant, and lightning fast as well,
> And it doesn't cost a nickel, so Bill Gates can go to hell."



-- 
Regards
Junwang Zhao


v5-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data

Re: make pg_ctl more friendly

[Junwang Zhao]

Hi Nathan,

On Tue, Jan 16, 2024 at 5:39 AM Nathan Bossart  wrote:
>
> +   POSTMASTER_RECOVERY_SHUTDOWN,
>
> Perhaps this should be POSTMASTER_SHUTDOWN_IN_RECOVERY to match the state
> in the control file?

Agreed

>
> +   case POSTMASTER_RECOVERY_SHUTDOWN:
> +   print_msg(_("PITR shutdown\n"));
> +   print_msg(_("update configuration for startup 
> again if needed\n"));
> +   break;
>
> I'm not sure I agree that this is a substantially friendlier message.  From
> a quick skim of the thread, it seems like you want to avoid sending a scary
> error message if Postgres was intentionally shut down while in recovery.
> If I got this particular message, I think I would be worried that something
> went wrong during my point-in-time restore, and I'd be scrambling to figure
> out what configuration this message wants me to update.
>
> If I'm correctly interpreting the intent here, it might be worth fleshing
> out the messages a bit more.  For example, instead of "PITR shutdown,"
> perhaps we could say "shut down while in recovery."

Make sense. Fixed. See V4

> And maybe we should
> point to the specific settings in the latter message.

I've changed this latter message to:
update recovery target settings for startup again if needed

What do you think?

>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com



-- 
Regards
Junwang Zhao


v4-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data

Re: speed up a logical replica setup

[Junwang Zhao]

Hi Euler,

On Fri, Jan 12, 2024 at 6:16 AM Euler Taveira  wrote:
>
> On Thu, Jan 11, 2024, at 9:18 AM, Hayato Kuroda (Fujitsu) wrote:
>
> I have been concerned that the patch has not been tested by cfbot due to the
> application error. Also, some comments were raised. Therefore, I created a 
> patch
> to move forward.
>
>
> Let me send an updated patch to hopefully keep the CF bot happy. The following
> items are included in this patch:
>
> * drop physical replication slot if standby is using one [1].
> * cleanup small changes (copyright, .gitignore) [2][3]
> * fix getopt_long() options [2]
> * fix format specifier for some messages
> * move doc to Server Application section [4]
> * fix assert failure
> * ignore duplicate database names [2]
> * store subscriber server log into a separate file
> * remove MSVC support
>
> I'm still addressing other reviews and I'll post another version that includes
> it soon.
>
> [1] 
> https://www.postgresql.org/message-id/e02a2c17-22e5-4ba6-b788-de696ab74f1e%40app.fastmail.com
> [2] 
> https://www.postgresql.org/message-id/CALDaNm1joke42n68LdegN5wCpaeoOMex2EHcdZrVZnGD3UhfNQ%40mail.gmail.com
> [3] 
> https://www.postgresql.org/message-id/TY3PR01MB98895BA6C1D72CB8582CACC4F5682%40TY3PR01MB9889.jpnprd01.prod.outlook.com
> [4] 
> https://www.postgresql.org/message-id/TY3PR01MB988978C7362A101927070D29F56A2%40TY3PR01MB9889.jpnprd01.prod.outlook.com
>

+ 
+  pg_subscriber
+  create a new logical replica from a standby server
+ 
I'm a bit confused about this wording because we are converting a standby
to a logical replica but not creating a new logical replica and leaving the
standby as is. How about:

convert a standby replica to a logical replica

+  
+   The pg_subscriber should be run at the target
+   server. The source server (known as publisher server) should accept logical
+   replication connections from the target server (known as subscriber server).
+   The target server should accept local logical replication connection.
+  

What is *local logical replication*? I can't find any clue in the patch, can you
give me some hint?


>
> --
> Euler Taveira
> EDB   https://www.enterprisedb.com/
>


-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

Hi,

On Wed, Jan 10, 2024 at 2:20 PM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Fri, 22 Dec 2023 10:58:05 +0800,
>   Junwang Zhao  wrote:
>
> >> 1. Add an opaque space for custom COPY TO handler
> >>* Add CopyToState{Get,Set}Opaque()
> >>
> >> https://github.com/kou/postgres/commit/5a610b6a066243f971e029432db67152cfe5e944
> >>
> >> 2. Export CopyToState::attnumlist
> >>* Add CopyToStateGetAttNumList()
> >>
> >> https://github.com/kou/postgres/commit/15fcba8b4e95afa86edb3f677a7bdb1acb1e7688
> >>
> >> 3. Export CopySend*()
> >>* Rename CopySend*() to CopyToStateSend*() and export them
> >>* Exception: CopySendEndOfRow() to CopyToStateFlush() because
> >>  it just flushes the internal buffer now.
> >>
> >> https://github.com/kou/postgres/commit/289a5640135bde6733a1b8e2c412221ad522901e
> >>
> > I guess the purpose of these helpers is to avoid expose CopyToState to
> > copy.h,
>
> Yes.
>
> > but I
> > think expose CopyToState to user might make life easier, users might want 
> > to use
> > the memory contexts of the structure (though I agree not all the
> > fields are necessary
> > for extension handers).
>
> OK. I don't object it as I said in another e-mail:
> https://www.postgresql.org/message-id/flat/20240110.120644.1876591646729327180.kou%40clear-code.com#d923173e9625c20319750155083cbd72
>
> >> 2. Need an opaque space like IndexScanDesc::opaque does
> >>
> >>* A custom COPY TO handler needs to keep its data
> >
> > I once thought users might want to parse their own options, maybe this
> > is a use case for this opaque space.
>
> Good catch! I forgot to suggest a callback for custom format
> options. How about the following API?
>
> 
> ...
> typedef bool (*CopyToProcessOption_function) (CopyToState cstate, DefElem 
> *defel);
>
> ...
> typedef bool (*CopyFromProcessOption_function) (CopyFromState cstate, DefElem 
> *defel);
>
> typedef struct CopyToFormatRoutine
> {
> ...
> CopyToProcessOption_function process_option_fn;
> } CopyToFormatRoutine;
>
> typedef struct CopyFromFormatRoutine
> {
> ...
> CopyFromProcessOption_function process_option_fn;
> } CopyFromFormatRoutine;
> 
>
> 
> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
> index e7597894bf..1aa8b62551 100644
> --- a/src/backend/commands/copy.c
> +++ b/src/backend/commands/copy.c
> @@ -416,6 +416,7 @@ void
>  ProcessCopyOptions(ParseState *pstate,
>CopyFormatOptions *opts_out,
>bool is_from,
> +  void *cstate, /* CopyToState* for 
> !is_from, CopyFromState* for is_from */
>List *options)
>  {
> boolformat_specified = false;
> @@ -593,11 +594,19 @@ ProcessCopyOptions(ParseState *pstate,
>  parser_errposition(pstate, 
> defel->location)));
> }
> else
> -   ereport(ERROR,
> -   (errcode(ERRCODE_SYNTAX_ERROR),
> -errmsg("option \"%s\" not 
> recognized",
> -   defel->defname),
> -parser_errposition(pstate, 
> defel->location)));
> +   {
> +   bool processed;
> +   if (is_from)
> +   processed = 
> opts_out->from_ops->process_option_fn(cstate, defel);
> +   else
> +   processed = 
> opts_out->to_ops->process_option_fn(cstate, defel);
> +   if (!processed)
> +   ereport(ERROR,
> +   
> (errcode(ERRCODE_SYNTAX_ERROR),
> +errmsg("option \"%s\" not 
> recognized",
> +   
> defel->defname),
> +parser_errposition(pstate, 
> defel->location)));
> +   }
> }
>
> /*
> 

Looks good.

>
> > For the name, I thought private_data might be a better candidate than
>

Re: make pg_ctl more friendly

[Junwang Zhao]

Hi Nazir,

On Tue, Jan 9, 2024 at 9:23 PM Nazir Bilal Yavuz  wrote:
>
> Hi,
>
> Thank you for working on this! I agree that the current message is not 
> friendly.
>
> On Thu, 9 Nov 2023 at 10:19, Junwang Zhao  wrote:
> >
> > On Thu, Nov 9, 2023 at 3:08 PM Junwang Zhao  wrote:
> > >
> > > After a PITR shutdown, the db state should be *shut down in recovery*, 
> > > try the
> > > patch attached.
> > >
> >
> > previous patch has some format issues, V2 attached.
>
> v2-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch:
>
> -   "Examine the log output.\n"),
> +   "Examine the log output\n"),
>   progname);
>
> I don't think that this is needed.
There seems to be no common sense for the ending dot when using
write_stderr, so I will leave this not changed.

>
> Other than that, the patch looks good and I confirm that after PITR shutdown:
>
> "PITR shutdown"
> "update configuration for startup again if needed"
>
> message shows up, instead of:
>
> "pg_ctl: could not start server"
> "Examine the log output.".
>
> nitpick: It would be better if the order of the error message cases
> and enums is the same ( i.e. 'POSTMASTER_RECOVERY_SHUTDOWN' before
> 'POSTMASTER_FAILED' in enum )
Agreed, fixed in V3

>
> --
> Regards,
> Nazir Bilal Yavuz
> Microsoft



-- 
Regards
Junwang Zhao


v3-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data

Re: Change GUC hashtable to use simplehash?

[Junwang Zhao]

Hi John,

On Mon, Jan 8, 2024 at 10:44 AM John Naylor  wrote:
>
> On Sat, Jan 6, 2024 at 9:01 AM jian he  wrote:
> >
> > latency average = 147.782 ms
> > select * from bench_cstring_hash_unaligned(10);
> > latency average = 101.179 ms
> > select * from bench_cstring_hash_aligned(10);
> > latency average = 101.219 ms
>
> Thanks for testing again! This looks closer to my results. It doesn't
> show improvement for the aligned case, but it's not worse, either.
>
> There is still some polishing to be done, mostly on comments/examples,
> but I think it's mostly there. I'll return to it by next week.
>
>

+ * Portions Copyright (c) 2018-2023, PostgreSQL Global Development Group

A kind reminder, it's already 2024 :)

I'm also curious why the 2018, is there any convention for that?

-- 
Regards
Junwang Zhao

Re: Transaction timeout

[Junwang Zhao]

On Fri, Dec 29, 2023 at 6:00 PM Andrey M. Borodin  wrote:
>
>
>
> > On 28 Dec 2023, at 21:02, Junwang Zhao  wrote:
> >
> > Seems V5~V17 doesn't work as expected for Nikolay's case:
> >
>
> Yeah, that's a problem.
> > So I propose the following change, what do you think?
> This breaks COMMIT AND CHAIN.
>
> PFA v18: I've added a test for Nik's case and for COMMIT AND CHAIN. Now we 
> need to fix stuff to pass this tests (I've crafted output).
> We also need test for patchset step "Try to enable transaction_timeout before 
> next command".
>
> Thanks!

After exploring the code, I found scheduling the timeout in
`StartTransaction` might be a reasonable idea, all the chain
commands will call this function.

What concerns me is that it is also called by StartParallelWorkerTransaction,
I'm not sure if we should enable this timeout for parallel execution.

Thought?

>
>
> Best regards, Andrey Borodin.



-- 
Regards
Junwang Zhao


v19-0002-Use-test-from-Li-Japin-Also-add-tests-for-multip.patch
Description: Binary data


v19-0003-Try-to-enable-transaction_timeout-before-next-co.patch
Description: Binary data


v19-0001-Introduce-transaction_timeout.patch
Description: Binary data


v19-0004-fix-reschedule-timeout-for-each-commmand.patch
Description: Binary data

Re: Transaction timeout

[Junwang Zhao]

Hey Andrey,

On Sun, Dec 24, 2023 at 1:14 AM Andrey M. Borodin  wrote:
>
>
>
> > On 22 Dec 2023, at 10:39, Japin Li  wrote:
> >
> >
> > I try to split the test for transaction timeout, and all passed on my CI 
> > [1].
>
>
> I like the refactoring you did in timeout.spec. I thought it is impossible, 
> because permutations would try to reinitialize FATALed sessions. But, 
> obviously, tests work the way you refactored it.
> However I don't think ignoring test failures on Windows without understanding 
> root cause is a good idea.
> Let's get back to v13 version of tests, understand why it failed, apply your 
> test refactorings afterwards. BTW are you sure that v14 refactorings are 
> functional equivalent of v13 tests?
>
> To go with this plan I attach slightly modified version of v13 tests in v16 
> patchset. The only change is timing in "sleep_there" step. I suspect that 
> failure was induced by more coarse timer granularity on Windows. Tests were 
> giving only 9 milliseconds for a timeout to entirely wipe away backend from 
> pg_stat_activity. This saves testing time, but might induce false positive 
> test flaps. So I've raised wait times to 100ms. This seems too much, but I do 
> not have other ideas how to ensure tests stability. Maybe 50ms would be 
> enough, I do not know. Isolation runs ~50 seconds now. I'm tempted to say 
> that 200ms for timeouts worth it.
>
>
> As to 2nd step "Try to enable transaction_timeout during transaction", I 
> think this makes sense. But if we are doing so, shouldn't we also allow to 
> enable idle_in_transaction timeout in a same manner? Currently we only allow 
> to disable other timeouts... Also, if we are already in transaction, 
> shouldn't we also subtract current transaction span from timeout?
> I think making this functionality as another step of the patchset was a good 
> idea.
>
> Thanks!
Seems V5~V17 doesn't work as expected for Nikolay's case:

postgres=# set transaction_timeout to '2s';
SET
postgres=# begin; select pg_sleep(1); select pg_sleep(1); select
pg_sleep(1); select pg_sleep(1); select pg_sleep(1); commit;
BEGIN

The reason for this seems the timer has been refreshed for each
command, xact_started along can not indicate it's a new
transaction or not, there is a TransactionState contains some
infos.

So I propose the following change, what do you think?

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index a2611cf8e6..cffd2c44d0 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -2746,7 +2746,7 @@ start_xact_command(void)
StartTransactionCommand();

/* Schedule or reschedule transaction timeout */
-   if (TransactionTimeout > 0)
+   if (TransactionTimeout > 0 &&
!get_timeout_active(TRANSACTION_TIMEOUT))
        enable_timeout_after(TRANSACTION_TIMEOUT,
TransactionTimeout);

xact_started = true;

>
>
> Best regards, Andrey Borodin.



-- 
Regards
Junwang Zhao

Re: Proposal to include --exclude-extension Flag in pg_dump

[Junwang Zhao]

Hi

On Mon, Dec 25, 2023 at 6:22 PM Ayush Vatsa  wrote:
>
> Added a CF entry for the same https://commitfest.postgresql.org/46/4721/
>
> Regards
> Ayush Vatsa
> Amazon Web Services (AWS)
>
> On Mon, 25 Dec 2023 at 15:48, Ayush Vatsa  wrote:
>>
>> Hi PostgreSQL Community,
>> Recently I have been working on pg_dump regarding my project and wanted to 
>> exclude an extension from the dump generated. I wonder why it doesn't have 
>> --exclude-extension type of support whereas --extension exists!
>> Since I needed that support, I took the initiative to contribute to the 
>> community by adding the --exclude-extension flag.
>> Attached is the patch for the same. Looking forward to your feedback.
>>
>> Regards
>> Ayush Vatsa
>> Amazon Web services (AWS)

  printf(_("  -e, --extension=PATTERN  dump the specified
extension(s) only\n"));
+ printf(_("  --exclude-extension=PATTERN  do NOT dump the specified
extension(s)\n"));
  printf(_("  -E, --encoding=ENCODING  dump the data in encoding
ENCODING\n"));

long options should not mess with short options, does the following
make sense to you?

 printf(_(" --enable-row-security enable row security (dump only
content user has\n"
 "access to)\n"));
+ printf(_("  --exclude-extension=PATTERN  do NOT dump the specified
extension(s)\n"));
 printf(_(" --exclude-table-and-children=PATTERN\n"

-- 
Regards
Junwang Zhao

Re: Transaction timeout

[Junwang Zhao]

Hi Andrey,

On Sun, Dec 24, 2023 at 1:14 AM Andrey M. Borodin  wrote:
>
>
>
> > On 22 Dec 2023, at 10:39, Japin Li  wrote:
> >
> >
> > I try to split the test for transaction timeout, and all passed on my CI 
> > [1].
>
>
> I like the refactoring you did in timeout.spec. I thought it is impossible, 
> because permutations would try to reinitialize FATALed sessions. But, 
> obviously, tests work the way you refactored it.
> However I don't think ignoring test failures on Windows without understanding 
> root cause is a good idea.
> Let's get back to v13 version of tests, understand why it failed, apply your 
> test refactorings afterwards. BTW are you sure that v14 refactorings are 
> functional equivalent of v13 tests?
>
> To go with this plan I attach slightly modified version of v13 tests in v16 
> patchset. The only change is timing in "sleep_there" step. I suspect that 
> failure was induced by more coarse timer granularity on Windows. Tests were 
> giving only 9 milliseconds for a timeout to entirely wipe away backend from 
> pg_stat_activity. This saves testing time, but might induce false positive 
> test flaps. So I've raised wait times to 100ms. This seems too much, but I do 
> not have other ideas how to ensure tests stability. Maybe 50ms would be 
> enough, I do not know. Isolation runs ~50 seconds now. I'm tempted to say 
> that 200ms for timeouts worth it.
>
>
> As to 2nd step "Try to enable transaction_timeout during transaction", I 
> think this makes sense. But if we are doing so, shouldn't we also allow to 
> enable idle_in_transaction timeout in a same manner? Currently we only allow 
> to disable other timeouts... Also, if we are already in transaction, 
> shouldn't we also subtract current transaction span from timeout?
idle_in_transaction_session_timeout is already the behavior Japin suggested,
it is enabled before backend sends *read for query* to client.

> I think making this functionality as another step of the patchset was a good 
> idea.
>
> Thanks!
>
>
> Best regards, Andrey Borodin.



-- 
Regards
Junwang Zhao

Re: Transaction timeout

[Junwang Zhao]

On Sat, Dec 23, 2023 at 11:17 AM Japin Li  wrote:
>
> a
> On Sat, 23 Dec 2023 at 10:40, Japin Li  wrote:
> > On Sat, 23 Dec 2023 at 08:32, Japin Li  wrote:
> >> On Fri, 22 Dec 2023 at 23:30, Junwang Zhao  wrote:
> >>> On Fri, Dec 22, 2023 at 10:44 PM Japin Li  wrote:
> >>>>
> >>>>
> >>>> On Fri, 22 Dec 2023 at 22:37, Junwang Zhao  wrote:
> >>>> > On Fri, Dec 22, 2023 at 10:25 PM Japin Li  wrote:
> >>>> >> I try to set idle_in_transaction_session_timeout after begin 
> >>>> >> transaction,
> >>>> >> it changes immediately, so I think transaction_timeout should also be 
> >>>> >> take
> >>>> >> immediately.
> >>>> >
> >>>> > Ah, right, idle_in_transaction_session_timeout is set after the set
> >>>> > command finishes and before the backend send *ready for query*
> >>>> > to the client, so the value of the GUC is already set before
> >>>> > next command.
> >>>> >
> >>>>
> >>>> I mean, is it possible to set transaction_timeout before next comand?
> >>>>
> >>> Yeah, it's possible, set transaction_timeout in the when it first
> >>> goes into *idle in transaction* mode, see the attached files.
> >>>
> >>
> >> Thanks for updating the patch, LGTM.
> >
> > Sorry for the noise!
> >
> > Read the previous threads, I find why the author enable transaction_timeout
> > in start_xact_command().
> >
> > The v15 patch cannot handle COMMIT AND CHAIN, see [1]. For example:
> >
> > SET transaction_timeout TO '2s'; BEGIN; SELECT 1, pg_sleep(1); COMMIT AND 
> > CHAIN; SELECT 2, pg_sleep(1); COMMIT;
> >
> > The transaction_timeout do not reset when executing COMMIT AND CHAIN.
> >
> > [1] 
> > https://www.postgresql.org/message-id/a906dea1-76a1-4f26-76c5-a7efad3ef5b8%40oss.nttdata.com
>
> Attach v16 to solve this. Any suggestions?

I've checked this with *COMMIT AND CHAIN* and *ABORT AND CHAIN*,
both work as expected. Thanks for the update.

>
> --
> Regrads,
> Japin Li
> ChengDu WenWu Information Technology Co., Ltd.
>


-- 
Regards
Junwang Zhao

Re: Transaction timeout

[Junwang Zhao]

On Sat, Dec 23, 2023 at 10:40 AM Japin Li  wrote:
>
>
> On Sat, 23 Dec 2023 at 08:32, Japin Li  wrote:
> > On Fri, 22 Dec 2023 at 23:30, Junwang Zhao  wrote:
> >> On Fri, Dec 22, 2023 at 10:44 PM Japin Li  wrote:
> >>>
> >>>
> >>> On Fri, 22 Dec 2023 at 22:37, Junwang Zhao  wrote:
> >>> > On Fri, Dec 22, 2023 at 10:25 PM Japin Li  wrote:
> >>> >> I try to set idle_in_transaction_session_timeout after begin 
> >>> >> transaction,
> >>> >> it changes immediately, so I think transaction_timeout should also be 
> >>> >> take
> >>> >> immediately.
> >>> >
> >>> > Ah, right, idle_in_transaction_session_timeout is set after the set
> >>> > command finishes and before the backend send *ready for query*
> >>> > to the client, so the value of the GUC is already set before
> >>> > next command.
> >>> >
> >>>
> >>> I mean, is it possible to set transaction_timeout before next comand?
> >>>
> >> Yeah, it's possible, set transaction_timeout in the when it first
> >> goes into *idle in transaction* mode, see the attached files.
> >>
> >
> > Thanks for updating the patch, LGTM.
>
> Sorry for the noise!
>
> Read the previous threads, I find why the author enable transaction_timeout
> in start_xact_command().
>
> The v15 patch cannot handle COMMIT AND CHAIN, see [1]. For example:

I didn't read the previous threads, sorry for that, let's stick to v14.

>
> SET transaction_timeout TO '2s'; BEGIN; SELECT 1, pg_sleep(1); COMMIT AND 
> CHAIN; SELECT 2, pg_sleep(1); COMMIT;
>
> The transaction_timeout do not reset when executing COMMIT AND CHAIN.
>
> [1] 
> https://www.postgresql.org/message-id/a906dea1-76a1-4f26-76c5-a7efad3ef5b8%40oss.nttdata.com
>
> --
> Regrads,
> Japin Li
> ChengDu WenWu Information Technology Co., Ltd.



-- 
Regards
Junwang Zhao

Re: Transaction timeout

[Junwang Zhao]

On Fri, Dec 22, 2023 at 10:44 PM Japin Li  wrote:
>
>
> On Fri, 22 Dec 2023 at 22:37, Junwang Zhao  wrote:
> > On Fri, Dec 22, 2023 at 10:25 PM Japin Li  wrote:
> >> I try to set idle_in_transaction_session_timeout after begin transaction,
> >> it changes immediately, so I think transaction_timeout should also be take
> >> immediately.
> >
> > Ah, right, idle_in_transaction_session_timeout is set after the set
> > command finishes and before the backend send *ready for query*
> > to the client, so the value of the GUC is already set before
> > next command.
> >
>
> I mean, is it possible to set transaction_timeout before next comand?
>
Yeah, it's possible, set transaction_timeout in the when it first
goes into *idle in transaction* mode, see the attached files.

>
> --
> Regrads,
> Japin Li
> ChengDu WenWu Information Technology Co., Ltd.



-- 
Regards
Junwang Zhao


v15-0001-Introduce-transaction_timeout.patch
Description: Binary data


v15-0002-set-transaction_timeout-before-next-command.patch
Description: Binary data

Re: Transaction timeout

[Junwang Zhao]

On Fri, Dec 22, 2023 at 10:25 PM Japin Li  wrote:
>
>
> On Fri, 22 Dec 2023 at 20:29, Junwang Zhao  wrote:
> > On Fri, Dec 22, 2023 at 1:39 PM Japin Li  wrote:
> >>
> >>
> >> On Tue, 19 Dec 2023 at 22:06, Japin Li  wrote:
> >> > On Tue, 19 Dec 2023 at 18:27, Andrey M. Borodin  
> >> > wrote:
> >> >>> On 19 Dec 2023, at 13:26, Andrey M. Borodin  
> >> >>> wrote:
> >> >>>
> >> >>> I don’t have Windows machine, so I hope CF bot will pick this.
> >> >>
> >> >> I used Github CI to produce version of tests that seems to be is stable 
> >> >> on Windows.
> >> >
> >> > It still failed on Windows Server 2019 [1].
> >> >
> >> > diff -w -U3 C:/cirrus/src/test/isolation/expected/timeouts.out 
> >> > C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out
> >> > --- C:/cirrus/src/test/isolation/expected/timeouts.out2023-12-19 
> >> > 10:34:30.354721100 +
> >> > +++ C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out  
> >> > 2023-12-19 10:38:25.877981600 +
> >> > @@ -100,7 +100,7 @@
> >> >  step stt3_check_stt2: SELECT count(*) FROM pg_stat_activity WHERE 
> >> > application_name = 'isolation/timeouts/stt2'
> >> >  count
> >> >  -
> >> > -0
> >> > +1
> >> >  (1 row)
> >> >
> >> >  step itt4_set: SET idle_in_transaction_session_timeout = '1ms'; SET 
> >> > statement_timeout = '10s'; SET lock_timeout = '10s'; SET 
> >> > transaction_timeout = '10s';
> >> >
> >> > [1] 
> >> > https://api.cirrus-ci.com/v1/artifact/task/4707530400595968/testrun/build/testrun/isolation/isolation/regression.diffs
> >>
> >> Hi,
> >>
> >> I try to split the test for transaction timeout, and all passed on my CI 
> >> [1].
> >>
> >> OTOH, I find if I set transaction_timeout in a transaction, it will not 
> >> take
> >> effect immediately.  For example:
> >>
> >> [local]:2049802 postgres=# BEGIN;
> >> BEGIN
> >> [local]:2049802 postgres=*# SET transaction_timeout TO '1s';
> > when this execute, TransactionTimeout is still 0, this command will
> > not set timeout
> >> SET
> >> [local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1;  -- wait 
> >> 10s
> > when this command get execute, start_xact_command will enable the timer
>
> Thanks for your exaplantion, got it.
>
> >>relname
> >> --
> >>  pg_statistic
> >> (1 row)
> >>
> >> [local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1;
> >> FATAL:  terminating connection due to transaction timeout
> >> server closed the connection unexpectedly
> >> This probably means the server terminated abnormally
> >> before or while processing the request.
> >> The connection to the server was lost. Attempting reset: Succeeded.
> >>
> >> It looks odd.  Does this is expected? I'm not read all the threads,
> >> am I missing something?
> >
> > I think this is by design, if you debug statement_timeout, it's the same
> > behaviour, the timeout will be set for each command after the second
> > command was called, you just aren't aware of this.
> >
>
> I try to set idle_in_transaction_session_timeout after begin transaction,
> it changes immediately, so I think transaction_timeout should also be take
> immediately.

Ah, right, idle_in_transaction_session_timeout is set after the set
command finishes and before the backend send *ready for query*
to the client, so the value of the GUC is already set before
next command.

I bet you must have checked this ;)

>
> > I doubt people will set this in a transaction.
>
> Maybe not,
>
>
> --
> Regrads,
> Japin Li
> ChengDu WenWu Information Technology Co., Ltd.



-- 
Regards
Junwang Zhao

Re: Transaction timeout

[Junwang Zhao]

On Fri, Dec 22, 2023 at 1:39 PM Japin Li  wrote:
>
>
> On Tue, 19 Dec 2023 at 22:06, Japin Li  wrote:
> > On Tue, 19 Dec 2023 at 18:27, Andrey M. Borodin  
> > wrote:
> >>> On 19 Dec 2023, at 13:26, Andrey M. Borodin  wrote:
> >>>
> >>> I don’t have Windows machine, so I hope CF bot will pick this.
> >>
> >> I used Github CI to produce version of tests that seems to be is stable on 
> >> Windows.
> >
> > It still failed on Windows Server 2019 [1].
> >
> > diff -w -U3 C:/cirrus/src/test/isolation/expected/timeouts.out 
> > C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out
> > --- C:/cirrus/src/test/isolation/expected/timeouts.out2023-12-19 
> > 10:34:30.354721100 +
> > +++ C:/cirrus/build/testrun/isolation/isolation/results/timeouts.out  
> > 2023-12-19 10:38:25.877981600 +
> > @@ -100,7 +100,7 @@
> >  step stt3_check_stt2: SELECT count(*) FROM pg_stat_activity WHERE 
> > application_name = 'isolation/timeouts/stt2'
> >  count
> >  -
> > -0
> > +1
> >  (1 row)
> >
> >  step itt4_set: SET idle_in_transaction_session_timeout = '1ms'; SET 
> > statement_timeout = '10s'; SET lock_timeout = '10s'; SET 
> > transaction_timeout = '10s';
> >
> > [1] 
> > https://api.cirrus-ci.com/v1/artifact/task/4707530400595968/testrun/build/testrun/isolation/isolation/regression.diffs
>
> Hi,
>
> I try to split the test for transaction timeout, and all passed on my CI [1].
>
> OTOH, I find if I set transaction_timeout in a transaction, it will not take
> effect immediately.  For example:
>
> [local]:2049802 postgres=# BEGIN;
> BEGIN
> [local]:2049802 postgres=*# SET transaction_timeout TO '1s';
when this execute, TransactionTimeout is still 0, this command will
not set timeout
> SET
> [local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1;  -- wait 10s
when this command get execute, start_xact_command will enable the timer
>relname
> --
>  pg_statistic
> (1 row)
>
> [local]:2049802 postgres=*# SELECT relname FROM pg_class LIMIT 1;
> FATAL:  terminating connection due to transaction timeout
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Succeeded.
>
> It looks odd.  Does this is expected? I'm not read all the threads,
> am I missing something?

I think this is by design, if you debug statement_timeout, it's the same
behaviour, the timeout will be set for each command after the second
command was called, you just aren't aware of this.

I doubt people will set this in a transaction.
>
> [1] https://cirrus-ci.com/build/6574686130143232
>
> --
> Regrads,
> Japin Li
> ChengDu WenWu Information Technology Co., Ltd.
>


-- 
Regards
Junwang Zhao

Re: [meson] expose buildtype debug/optimization info to pg_config

[Junwang Zhao]

Hi,

On Fri, Dec 15, 2023 at 10:20 PM Andres Freund  wrote:
>
> Hi,
>
> On 2023-12-14 17:24:58 +0800, Junwang Zhao wrote:
> > On Thu, Dec 14, 2023 at 4:50 PM Peter Eisentraut  
> > wrote:
> > >
> > > On 12.12.23 11:40, Junwang Zhao wrote:
> > > > build system using configure set VAL_CFLAGS with debug and
> > > > optimization flags, so pg_config will show these infos. Some
> > > > extensions depend on the mechanism.
> > > >
> > > > This patch exposes these flags with a typo fixed together.
> > >
> > > I have committed the typo fix.
> > >
> > > But I would like to learn more about the requirements of extensions in
> > > this area.  This seems a bit suspicious to me.
> >
> > This is what I found when building citus against an installation
> > of meson debug build pg instance, since the CFLAGS doesn't
> > contain -g flag, the binary doesn't include the debug information,
> > which is different behavior from configure building system.
>
> Hm. I'm not sure it's the right call to make extensions build the same way as
> the main postgres install with regard to optimization and debug info. So I
> feel a bit hesitant around generating -g and particularly -Ox. But it's
> historically what we've done...
>
> If we want to do so, I think this should not check buildtype, but debug.

I'm confused which *debug* do you mean, can you be more specific?
>
>
> > Another issue I found is that some C++
> > extensions(ajust/parquet_fdw for example) don't build against
> > the meson generated pgxs.mk, since it doesn't set the CXX
> > command. CXX is only set when llvm option is enabled, which
> > is different from old building system.
>
> I wanted to skip the C++ tests when we don't need C++, because it makes
> configure take longer. But I could be convinced that we should always at least
> determine the C++ compiler for Makefile.global.

The first idea that came to my mind is using the *project* command
to set [`c`, `cpp`], but this might be a little bit confusing for somebody.

Then I tried another way by adding a 'pgxscpp' option to let the user
choose whether he will set the C++ compiler for Makefile.global.
It works but may not be an ideal way, see the attached.


>
> Greetings,
>
> Andres Freund



-- 
Regards
Junwang Zhao


0001-PGXS-determine-C-compiler-for-Makefile.global.patch
Description: Binary data

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Thu, Dec 21, 2023 at 5:35 PM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Mon, 11 Dec 2023 23:31:29 +0900,
>   Masahiko Sawada  wrote:
>
> > I've sketched the above idea including a test module in
> > src/test/module/test_copy_format, based on v2 patch. It's not splitted
> > and is dirty so just for discussion.
>
> I implemented a sample COPY TO handler for Apache Arrow that
> supports only integer and text.
>
> I needed to extend the patch:
>
> 1. Add an opaque space for custom COPY TO handler
>* Add CopyToState{Get,Set}Opaque()
>
> https://github.com/kou/postgres/commit/5a610b6a066243f971e029432db67152cfe5e944
>
> 2. Export CopyToState::attnumlist
>* Add CopyToStateGetAttNumList()
>
> https://github.com/kou/postgres/commit/15fcba8b4e95afa86edb3f677a7bdb1acb1e7688
>
> 3. Export CopySend*()
>* Rename CopySend*() to CopyToStateSend*() and export them
>* Exception: CopySendEndOfRow() to CopyToStateFlush() because
>  it just flushes the internal buffer now.
>
> https://github.com/kou/postgres/commit/289a5640135bde6733a1b8e2c412221ad522901e
>
I guess the purpose of these helpers is to avoid expose CopyToState to
copy.h, but I
think expose CopyToState to user might make life easier, users might want to use
the memory contexts of the structure (though I agree not all the
fields are necessary
for extension handers).

> The attached patch is based on the Sawada-san's patch and
> includes the above changes. Note that this patch is also
> dirty so just for discussion.
>
> My suggestions from this experience:
>
> 1. Split COPY handler to COPY TO handler and COPY FROM handler
>
>* CopyFormatRoutine is a bit tricky. An extension needs
>  to create a CopyFormatRoutine node and
>  a CopyToFormatRoutine node.
>
>* If we just require "copy_to_${FORMAT}(internal)"
>  function and "copy_from_${FORMAT}(internal)" function,
>  we can remove the tricky approach. And it also avoid
>  name collisions with other handler such as tablesample
>  handler.
>  See also:
>  
> https://www.postgresql.org/message-id/flat/20231214.184414.2179134502876898942.kou%40clear-code.com#af71f364d0a9f5c144e45b447e5c16c9
>
> 2. Need an opaque space like IndexScanDesc::opaque does
>
>* A custom COPY TO handler needs to keep its data

I once thought users might want to parse their own options, maybe this
is a use case for this opaque space.

For the name, I thought private_data might be a better candidate than
opaque, but I do not insist.
>
> 3. Export CopySend*()
>
>* If we like minimum API, we just need to export
>  CopySendData() and CopySendEndOfRow(). But
>  CopySend{String,Char,Int32,Int16}() will be convenient
>  custom COPY TO handlers. (A custom COPY TO handler for
>  Apache Arrow doesn't need them.)

Do you use the arrow library to control the memory? Is there a way that
we can let the arrow use postgres' memory context? I'm not sure this
is necessary, just raise the question for discussion.
>
> Questions:
>
> 1. What value should be used for "format" in
>PgMsg_CopyOutResponse message?
>
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob;f=src/backend/commands/copyto.c;h=c66a047c4a79cc614784610f385f1cd0935350f3;hb=9ca6e7b9411e36488ef539a2c1f6846ac92a7072#l144
>
>It's 1 for binary format and 0 for text/csv format.
>
>Should we make it customizable by custom COPY TO handler?
>If so, what value should be used for this?
>
> 2. Do we need more tries for design discussion for the first
>    implementation? If we need, what should we try?
>
>
> Thanks,
> --
> kou

+PG_FUNCTION_INFO_V1(copy_testfmt_handler);
+Datum
+copy_testfmt_handler(PG_FUNCTION_ARGS)
+{
+ bool is_from = PG_GETARG_BOOL(0);
+ CopyFormatRoutine *cp = makeNode(CopyFormatRoutine);;
+

extra semicolon.

-- 
Regards
Junwang Zhao

Re: Transaction timeout

[Junwang Zhao]

On Wed, Dec 20, 2023 at 9:58 AM Thomas wen
 wrote:
>
> Hi Junwang Zhao
>  #should we invalidate lock_timeout? Or maybe just document this.
> I think you mean when lock_time is greater than trasaction-time invalidate 
> lock_timeout or  needs to be logged ?
>
I mean the interleaving of the gucs, which is lock_timeout and the new
introduced transaction_timeout,
if lock_timeout >= transaction_timeout, seems no need to enable lock_timeout.
>
>
>
> Best whish
> ____
> 发件人: Junwang Zhao 
> 发送时间: 2023年12月20日 9:48
> 收件人: Andrey M. Borodin 
> 抄送: Japin Li ; 邱宇航 ; Fujii Masao 
> ; Andrey Borodin ; Andres 
> Freund ; Michael Paquier ; 
> Nikolay Samokhvalov ; pgsql-hackers 
> ; pgsql-hackers@lists.postgresql.org 
> 
> 主题: Re: Transaction timeout
>
> On Tue, Dec 19, 2023 at 10:51 PM Junwang Zhao  wrote:
> >
> > On Tue, Dec 19, 2023 at 6:27 PM Andrey M. Borodin  
> > wrote:
> > >
> > >
> > >
> > > > On 19 Dec 2023, at 13:26, Andrey M. Borodin  
> > > > wrote:
> > > >
> > > > I don’t have Windows machine, so I hope CF bot will pick this.
> > >
> > > I used Github CI to produce version of tests that seems to be is stable 
> > > on Windows.
> > > Sorry for the noise.
> > >
> > >
> > > Best regards, Andrey Borodin.
> >
> > +   
> > +If transaction_timeout is shorter than
> > +idle_in_transaction_session_timeout or
> > statement_timeout
> > +transaction_timeout will invalidate longer 
> > timeout.
> > +   
> >
> > When transaction_timeout is *equal* to idle_in_transaction_session_timeout
> > or statement_timeout, idle_in_transaction_session_timeout and 
> > statement_timeout
> > will also be invalidated, the logic in the code seems right, though
> > this document
> > is a little bit inaccurate.
> >
>
> Unlike statement_timeout, this timeout can only 
> occur
> while waiting for locks.  Note that if
> statement_timeout
> is nonzero, it is rather pointless to set
> lock_timeout to
> the same or larger value, since the statement timeout would always
> trigger first.  If log_min_error_statement is set 
> to
> ERROR or lower, the statement that timed out will 
> be
>     logged.
>    
>
> There is a note about statement_timeout and lock_timeout, set both
> and lock_timeout >= statement_timeout is pointless, but this logic seems not
> implemented in the code. I am wondering if lock_timeout >= 
> transaction_timeout,
> should we invalidate lock_timeout? Or maybe just document this.
>
> > --
> > Regards
> > Junwang Zhao
>
>
>
> --
> Regards
> Junwang Zhao
>
>


-- 
Regards
Junwang Zhao

Re: Transaction timeout

[Junwang Zhao]

On Tue, Dec 19, 2023 at 10:51 PM Junwang Zhao  wrote:
>
> On Tue, Dec 19, 2023 at 6:27 PM Andrey M. Borodin  
> wrote:
> >
> >
> >
> > > On 19 Dec 2023, at 13:26, Andrey M. Borodin  wrote:
> > >
> > > I don’t have Windows machine, so I hope CF bot will pick this.
> >
> > I used Github CI to produce version of tests that seems to be is stable on 
> > Windows.
> > Sorry for the noise.
> >
> >
> > Best regards, Andrey Borodin.
>
> +   
> +If transaction_timeout is shorter than
> +idle_in_transaction_session_timeout or
> statement_timeout
> +transaction_timeout will invalidate longer 
> timeout.
> +   
>
> When transaction_timeout is *equal* to idle_in_transaction_session_timeout
> or statement_timeout, idle_in_transaction_session_timeout and 
> statement_timeout
> will also be invalidated, the logic in the code seems right, though
> this document
> is a little bit inaccurate.
>
   
Unlike statement_timeout, this timeout can only occur
while waiting for locks.  Note that if
statement_timeout
is nonzero, it is rather pointless to set
lock_timeout to
the same or larger value, since the statement timeout would always
trigger first.  If log_min_error_statement is set to
ERROR or lower, the statement that timed out will be
logged.
   

There is a note about statement_timeout and lock_timeout, set both
and lock_timeout >= statement_timeout is pointless, but this logic seems not
implemented in the code. I am wondering if lock_timeout >= transaction_timeout,
should we invalidate lock_timeout? Or maybe just document this.

> --
> Regards
> Junwang Zhao



-- 
Regards
Junwang Zhao

Re: Transaction timeout

[Junwang Zhao]

On Tue, Dec 19, 2023 at 6:27 PM Andrey M. Borodin  wrote:
>
>
>
> > On 19 Dec 2023, at 13:26, Andrey M. Borodin  wrote:
> >
> > I don’t have Windows machine, so I hope CF bot will pick this.
>
> I used Github CI to produce version of tests that seems to be is stable on 
> Windows.
> Sorry for the noise.
>
>
> Best regards, Andrey Borodin.

+   
+If transaction_timeout is shorter than
+idle_in_transaction_session_timeout or
statement_timeout
+transaction_timeout will invalidate longer timeout.
+   

When transaction_timeout is *equal* to idle_in_transaction_session_timeout
or statement_timeout, idle_in_transaction_session_timeout and statement_timeout
will also be invalidated, the logic in the code seems right, though
this document
is a little bit inaccurate.

-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Fri, Dec 15, 2023 at 12:45 PM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Fri, 15 Dec 2023 11:27:30 +0800,
>   Junwang Zhao  wrote:
>
> >> > Adding a prefix or suffix would be one option but to give extensions
> >> > more flexibility, another option would be to support format = 'custom'
> >> > and add the "handler" option to specify a copy handler function name
> >> > to call. For example, COPY ... FROM ... WITH (FORMAT = 'custom',
> >> > HANDLER = 'arrow_copy_handler').
> >>
> > I like the prefix/suffix idea, easy to implement. *custom* is not a FORMAT,
> > and user has to know the name of the specific handler names, not
> > intuitive.
>
> Ah! I misunderstood this idea. "custom" is the special
> format to use "HANDLER". I thought that we can use it like
>
>(FORMAT = 'arrow', HANDLER = 'arrow_copy_handler_impl1')
>
> and
>
>(FORMAT = 'arrow', HANDLER = 'arrow_copy_handler_impl2')
>
> .
>
> >> Interesting. If we use this option, users can choose an COPY
> >> FORMAT implementation they like from multiple
> >> implementations. For example, a developer may implement a
> >> COPY FROM FORMAT = 'json' handler with PostgreSQL's JSON
> >> related API and another developer may implement a handler
> >> with simdjson[1] which is a fast JSON parser. Users can
> >> choose whichever they like.
> > Not sure about this, why not move Json copy handler to contrib
> > as an example for others, any extensions share the same format
> > function name and just install one? No bound would implement
> > another CSV or TEXT copy handler IMHO.
>
> I should have used a different format not JSON as an example
> for easy to understand. I just wanted to say that extension
> developers can implement another implementation without
> conflicting another implementation.

Yeah, I can see the value of the HANDLER option now. The possibility
of two extensions for the same format using same hanlder name should
be rare I guess ;)
>
>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Fri, Dec 15, 2023 at 8:53 AM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Fri, 15 Dec 2023 05:19:43 +0900,
>   Masahiko Sawada  wrote:
>
> > To avoid collisions, extensions can be created in a
> > different schema than public.
>
> Thanks. I didn't notice it.
>
> > And note that built-in format copy handler doesn't need to
> > declare its handler function.
>
> Right. I know it.
>
> > Adding a prefix or suffix would be one option but to give extensions
> > more flexibility, another option would be to support format = 'custom'
> > and add the "handler" option to specify a copy handler function name
> > to call. For example, COPY ... FROM ... WITH (FORMAT = 'custom',
> > HANDLER = 'arrow_copy_handler').
>
I like the prefix/suffix idea, easy to implement. *custom* is not a FORMAT,
and user has to know the name of the specific handler names, not
intuitive.

> Interesting. If we use this option, users can choose an COPY
> FORMAT implementation they like from multiple
> implementations. For example, a developer may implement a
> COPY FROM FORMAT = 'json' handler with PostgreSQL's JSON
> related API and another developer may implement a handler
> with simdjson[1] which is a fast JSON parser. Users can
> choose whichever they like.
Not sure about this, why not move Json copy handler to contrib
as an example for others, any extensions share the same format
function name and just install one? No bound would implement
another CSV or TEXT copy handler IMHO.
>
> But specifying HANDLER = '...' explicitly is a bit
> inconvenient. Because only one handler will be installed in
> most use cases. In the case, users don't need to choose one
> handler.
>
> If we choose this option, it may be better that we also
> provide a mechanism that can work without HANDLER. Searching
> a function by name like tablesample method does is an option.
>
>
> [1]: https://github.com/simdjson/simdjson
>
>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao

Re: [meson] expose buildtype debug/optimization info to pg_config

[Junwang Zhao]

Hi Peter,

Thanks for looking into this.

On Thu, Dec 14, 2023 at 4:50 PM Peter Eisentraut  wrote:
>
> On 12.12.23 11:40, Junwang Zhao wrote:
> > build system using configure set VAL_CFLAGS with debug and
> > optimization flags, so pg_config will show these infos. Some
> > extensions depend on the mechanism.
> >
> > This patch exposes these flags with a typo fixed together.
>
> I have committed the typo fix.
>
> But I would like to learn more about the requirements of extensions in
> this area.  This seems a bit suspicious to me.

This is what I found when building citus against an installation
of meson debug build pg instance, since the CFLAGS doesn't
contain -g flag, the binary doesn't include the debug information,
which is different behavior from configure building system.

Another issue I found is that some C++
extensions(ajust/parquet_fdw for example) don't build against
the meson generated pgxs.mk, since it doesn't set the CXX
command. CXX is only set when llvm option is enabled, which
is different from old building system.

I don't insist we make Meson the same behaviour with old building
system, I just think the issues I raised might stop developers try
the fancy new building system. And the fix I post might not be
ideal, you and Andres might have better solutions.

>


-- 
Regards
Junwang Zhao

Re: Simplify newNode()

[Junwang Zhao]

On Thu, Dec 14, 2023 at 9:34 AM Zhang Mingli  wrote:
>
> Hi,
>
> LGTM.
>
> + Assert(size >= sizeof(Node)); /* need the tag, at least */
> + result = (Node *) palloc0fast(size);
> + result->type = tag;
>
> + return result;
> +}
>
> How about moving the comments /* need the tag, at least */ after result->type 
> = tag; by the way?

I don't think so, the comment has the meaning of the requested size
should at least the size
of Node, which contains just a NodeTag.

typedef struct Node
{
NodeTag type;
} Node;

>
>
>
> Zhang Mingli
> www.hashdata.xyz



-- 
Regards
Junwang Zhao

[meson] expose buildtype debug/optimization info to pg_config

[Junwang Zhao]

build system using configure set VAL_CFLAGS with debug and
optimization flags, so pg_config will show these infos. Some
extensions depend on the mechanism.

This patch exposes these flags with a typo fixed together.

-- 
Regards
Junwang Zhao


0001-meson-expose-buildtype-debug-optimization-info-to-pg.patch
Description: Binary data

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Mon, Dec 11, 2023 at 10:32 PM Masahiko Sawada  wrote:
>
> On Mon, Dec 11, 2023 at 7:19 PM Michael Paquier  wrote:
> >
> > On Mon, Dec 11, 2023 at 10:57:15AM +0900, Masahiko Sawada wrote:
> > > IIUC we cannot create two same name functions with the same arguments
> > > but a different return value type in the first place. It seems to me
> > > to be an overkill to change such a design.
> >
> > Agreed to not touch the logictics of LookupFuncName() for the sake of
> > this thread.  I have not checked the SQL specification, but I recall
> > that there are a few assumptions from the spec embedded in the lookup
> > logic particularly when it comes to specify a procedure name without
> > arguments.
> >
> > > Another idea is to encapsulate copy_to/from_handler by a super class
> > > like copy_handler. The handler function is called with an argument,
> > > say copyto, and returns copy_handler encapsulating either
> > > copy_to/from_handler depending on the argument.
> >
> > Yep, that's possible as well and can work as a cross-check between the
> > argument and the NodeTag assigned to the handler structure returned by
> > the function.
> >
> > At the end, the final result of the patch should IMO include:
> > - Documentation about how one can register a custom copy_handler.
> > - Something in src/test/modules/, minimalistic still useful that can
> > be used as a template when one wants to implement their own handler.
> > The documentation should mention about this module.
> > - No need for SQL functions for all the in-core handlers: let's just
> > return pointers to them based on the options given.
>
> Agreed.
>
> > It would be probably cleaner to split the patch so as the code is
> > refactored and evaluated with the in-core handlers first, and then
> > extended with the pluggable facilities and the function lookups.
>
> Agreed.
>
> I've sketched the above idea including a test module in
> src/test/module/test_copy_format, based on v2 patch. It's not splitted
> and is dirty so just for discussion.
>
The test_copy_format extension doesn't use the fields of CopyToState and
CopyFromState in this patch, I think we should move CopyFromStateData
and CopyToStateData to commands/copy.h, what do you think?

The framework in the patch LGTM.

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



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Sat, Dec 9, 2023 at 7:38 PM Hannu Krosing  wrote:
>
> Hi Junwang
>
> Please also see my presentation slides from last years PostgreSQL
> Conference in Berlin (attached)

I read through the slides, really promising ideas, it's will be great
if we can get there at last.

>
> The main Idea is to make not just "format", but also "transport" and
> "stream processing" extendable via virtual function tables.
The code is really coupled, it is not easy to do all of these in one round,
it will be great if you have a POC patch.

>
> Btw, will any of you here be in Prague next week ?
> Would be a good opportunity to discuss this in person.
Sorry, no.

>
>
> Best Regards
> Hannu
>
> On Sat, Dec 9, 2023 at 9:39 AM Junwang Zhao  wrote:
> >
> > On Sat, Dec 9, 2023 at 10:43 AM Hayato Kuroda (Fujitsu)
> >  wrote:
> > >
> > > Dear Junagn, Sutou-san,
> > >
> > > Basically I agree your point - improving a extendibility is good.
> > > (I remember that this theme was talked at Japan PostgreSQL conference)
> > > Below are my comments for your patch.
> > >
> > > 01. General
> > >
> > > Just to confirm - is it OK to partially implement APIs? E.g., only COPY 
> > > TO is
> > > available. Currently it seems not to consider a case which is not 
> > > implemented.
> > >
> > For partially implements, we can leave the hook as NULL, and check the NULL
> > at *ProcessCopyOptions* and report error if not supported.
> >
> > > 02. General
> > >
> > > It might be trivial, but could you please clarify how users can extend? 
> > > Is it OK
> > > to do below steps?
> > >
> > > 1. Create a handler function, via CREATE FUNCTION,
> > > 2. Register a handler, via new SQL (CREATE COPY HANDLER),
> > > 3. Specify the added handler as COPY ... FORMAT clause.
> > >
> > My original thought was option 2, but as Michael point, option 1 is
> > the right way
> > to go.
> >
> > > 03. General
> > >
> > > Could you please add document-related tasks to your TODO? I imagined like
> > > fdwhandler.sgml.
> > >
> > > 04. General - copyright
> > >
> > > For newly added files, the below copyright seems sufficient. See 
> > > applyparallelworker.c.
> > >
> > > ```
> > >  * Copyright (c) 2023, PostgreSQL Global Development Group
> > > ```
> > >
> > > 05. src/include/catalog/* files
> > >
> > > IIUC, 8000 or higher OIDs should be used while developing a patch. 
> > > src/include/catalog/unused_oids
> > > would suggest a candidate which you can use.
> >
> > Yeah, I will run renumber_oids.pl at last.
> >
> > >
> > > 06. copy.c
> > >
> > > I felt that we can create files per copying methods, like 
> > > copy_{text|csv|binary}.c,
> > > like indexes.
> > > How do other think?
> >
> > Not sure about this, it seems others have put a lot of effort into
> > splitting TO and From.
> > Also like to hear from others.
> >
> > >
> > > 07. fmt_to_name()
> > >
> > > I'm not sure the function is really needed. Can we follow like 
> > > get_foreign_data_wrapper_oid()
> > > and remove the funciton?
> >
> > I have referenced some code from greenplum, will remove this.
> >
> > >
> > > 08. GetCopyRoutineByName()
> > >
> > > Should we use syscache for searching a catalog?
> > >
> > > 09. CopyToFormatTextSendEndOfRow(), CopyToFormatBinaryStart()
> > >
> > > Comments still refer CopyHandlerOps, whereas it was renamed.
> > >
> > > 10. copy.h
> > >
> > > Per foreign.h and fdwapi.h, should we add a new header file and move some 
> > > APIs?
> > >
> > > 11. copy.h
> > >
> > > ```
> > > -/* These are private in commands/copy[from|to].c */
> > > -typedef struct CopyFromStateData *CopyFromState;
> > > -typedef struct CopyToStateData *CopyToState;
> > > ```
> > >
> > > Are above changes really needed?
> > >
> > > 12. CopyFormatOptions
> > >
> > > Can we remove `bool binary` in future?
> > >
> > > 13. external functions
> > >
> > > ```
> > > +extern void CopyToFormatTextStart(CopyToState cstate, TupleDesc tupDesc);
> > > +extern void CopyToFormatTextOneRow(CopyToState cstate, TupleTableSlot 
> > > *slo

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Sun, Dec 10, 2023 at 4:44 AM Sutou Kouhei  wrote:
>
> Hi,
>
> Thanks for reviewing our latest patch!
>
> In
>  
> 
>   "RE: Make COPY format extendable: Extract COPY TO format implementations" 
> on Sat, 9 Dec 2023 02:43:49 +,
>   "Hayato Kuroda (Fujitsu)"  wrote:
>
> > (I remember that this theme was talked at Japan PostgreSQL conference)
>
> Yes. I should have talked to you more at the conference...
> I will do it next time!
>
>
> Can we discuss how to proceed this improvement?
>
> There are 2 approaches for it:
>
> 1. Do the followings concurrently:
>a. Implementing small changes that got a consensus and
>   merge them step-by-step
>   (e.g. We got a consensus that we need to extract the
>   current format related routines.)
>b. Discuss design
>
>(v1-v3 patches use this approach.)
>
> 2. Implement one (large) complete patch set with design
>discussion and merge it
>
>(v4- patches use this approach.)
>
> Which approach is preferred? (Or should we choose another
> approach?)
>
> I thought that 1. is preferred because it will reduce review
> cost. So I chose 1.
>
> If 2. is preferred, I'll use 2. (I'll add more changes to
> the latest patch.)
>
I'm ok with both, and I'd like to work with you for the parquet
extension, excited about this new feature, thanks for bringing
this up.

Forgive me for making so much noise about approach 2, I
just want to hear about more suggestions of the final shape
of this feature.

>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Sat, Dec 9, 2023 at 10:43 AM Hayato Kuroda (Fujitsu)
 wrote:
>
> Dear Junagn, Sutou-san,
>
> Basically I agree your point - improving a extendibility is good.
> (I remember that this theme was talked at Japan PostgreSQL conference)
> Below are my comments for your patch.
>
> 01. General
>
> Just to confirm - is it OK to partially implement APIs? E.g., only COPY TO is
> available. Currently it seems not to consider a case which is not implemented.
>
For partially implements, we can leave the hook as NULL, and check the NULL
at *ProcessCopyOptions* and report error if not supported.

> 02. General
>
> It might be trivial, but could you please clarify how users can extend? Is it 
> OK
> to do below steps?
>
> 1. Create a handler function, via CREATE FUNCTION,
> 2. Register a handler, via new SQL (CREATE COPY HANDLER),
> 3. Specify the added handler as COPY ... FORMAT clause.
>
My original thought was option 2, but as Michael point, option 1 is
the right way
to go.

> 03. General
>
> Could you please add document-related tasks to your TODO? I imagined like
> fdwhandler.sgml.
>
> 04. General - copyright
>
> For newly added files, the below copyright seems sufficient. See 
> applyparallelworker.c.
>
> ```
>  * Copyright (c) 2023, PostgreSQL Global Development Group
> ```
>
> 05. src/include/catalog/* files
>
> IIUC, 8000 or higher OIDs should be used while developing a patch. 
> src/include/catalog/unused_oids
> would suggest a candidate which you can use.

Yeah, I will run renumber_oids.pl at last.

>
> 06. copy.c
>
> I felt that we can create files per copying methods, like 
> copy_{text|csv|binary}.c,
> like indexes.
> How do other think?

Not sure about this, it seems others have put a lot of effort into
splitting TO and From.
Also like to hear from others.

>
> 07. fmt_to_name()
>
> I'm not sure the function is really needed. Can we follow like 
> get_foreign_data_wrapper_oid()
> and remove the funciton?

I have referenced some code from greenplum, will remove this.

>
> 08. GetCopyRoutineByName()
>
> Should we use syscache for searching a catalog?
>
> 09. CopyToFormatTextSendEndOfRow(), CopyToFormatBinaryStart()
>
> Comments still refer CopyHandlerOps, whereas it was renamed.
>
> 10. copy.h
>
> Per foreign.h and fdwapi.h, should we add a new header file and move some 
> APIs?
>
> 11. copy.h
>
> ```
> -/* These are private in commands/copy[from|to].c */
> -typedef struct CopyFromStateData *CopyFromState;
> -typedef struct CopyToStateData *CopyToState;
> ```
>
> Are above changes really needed?
>
> 12. CopyFormatOptions
>
> Can we remove `bool binary` in future?
>
> 13. external functions
>
> ```
> +extern void CopyToFormatTextStart(CopyToState cstate, TupleDesc tupDesc);
> +extern void CopyToFormatTextOneRow(CopyToState cstate, TupleTableSlot *slot);
> +extern void CopyToFormatTextEnd(CopyToState cstate);
> +extern void CopyFromFormatTextStart(CopyFromState cstate, TupleDesc tupDesc);
> +extern bool CopyFromFormatTextNext(CopyFromState cstate, ExprContext 
> *econtext,
> +
> Datum *values, bool *nulls);
> +extern void CopyFromFormatTextErrorCallback(CopyFromState cstate);
> +
> +extern void CopyToFormatBinaryStart(CopyToState cstate, TupleDesc tupDesc);
> +extern void CopyToFormatBinaryOneRow(CopyToState cstate, TupleTableSlot 
> *slot);
> +extern void CopyToFormatBinaryEnd(CopyToState cstate);
> +extern void CopyFromFormatBinaryStart(CopyFromState cstate, TupleDesc 
> tupDesc);
> +extern bool CopyFromFormatBinaryNext(CopyFromState cstate,
> ExprContext *econtext,
> +
>   Datum *values, bool *nulls);
> +extern void CopyFromFormatBinaryErrorCallback(CopyFromState cstate);
> ```
>
> FYI - If you add files for {text|csv|binary}, these declarations can be 
> removed.
>
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
>

Thanks for all the valuable suggestions.

-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Fri, Dec 8, 2023 at 3:27 AM Masahiko Sawada  wrote:
>
> On Fri, Dec 8, 2023 at 1:39 AM Andrew Dunstan  wrote:
> >
> >
> > On 2023-12-07 Th 03:37, Junwang Zhao wrote:
> > >
> > > The point of this refactor (from my view) is to make it possible to add 
> > > new
> > > copy handlers in extensions, just like access method. As Andres suggested,
> > > a system catalog like *pg_copy_handler*, if we split TO and FROM into two
> > > sets of routines, does that mean we have to create two catalog(
> > > pg_copy_from_handler and pg_copy_to_handler)?
> >
> >
> >
> > Surely not. Either have two fields, one for the TO handler and one for
> > the FROM handler, or a flag on each row indicating if it's a FROM or TO
> > handler.

If we wrap the two fields into a single structure, that will still be in
copy.h, which I think is not necessary. A single routing wrapper should
be enough, the actual implementation still stays separate
copy_[to/from].c files.

>
> True.
>
> But why do we need a system catalog like pg_copy_handler in the first
> place? I imagined that an extension can define a handler function
> returning a set of callbacks and the parser can lookup the handler
> function by name, like FDW and TABLESAMPLE.
>
I can see FDW related utility commands but no TABLESAMPLE related,
and there is a pg_foreign_data_wrapper system catalog which has
a *fdwhandler* field.

If we want extensions to create a new copy handler, I think
something like pg_copy_hander should be necessary.

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

I go one step further to implement the pg_copy_handler, attached V5 is
the implementation with some changes suggested by Kou.

You can also review this on this github pull request [1].

[1]: https://github.com/zhjwpku/postgres/pull/1/files

-- 
Regards
Junwang Zhao


v5-0001-Extract-COPY-handlers.patch
Description: Binary data

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Thu, Dec 7, 2023 at 1:05 PM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Wed, 6 Dec 2023 22:07:51 +0800,
>   Junwang Zhao  wrote:
>
> > Should we extract both *copy to* and *copy from* for the first step, in that
> > case we can add the pg_copy_handler catalog smoothly later.
>
> I don't object it (mixing TO/FROM changes to one patch) but
> it may make review difficult. Is it acceptable?
>
> FYI: I planed that I implement TO part, and then FROM part,
> and then unify TO/FROM parts if needed. [1]

I'm fine with step by step refactoring, let's just wait for more
suggestions.

>
> > Attached V4 adds 'extract copy from' and it passed the cirrus ci,
> > please take a look.
>
> Thanks. Here are my comments:
>
> > + /*
> > + * Error is relevant to a particular line.
> > + *
> > + * If line_buf still contains the correct line, print 
> > it.
> > + */
> > + if (cstate->line_buf_valid)
>
> We need to fix the indentation.
>
> > +CopyFromFormatBinaryStart(CopyFromState cstate, TupleDesc tupDesc)
> > +{
> > + FmgrInfo   *in_functions;
> > + Oid*typioparams;
> > + Oid in_func_oid;
> > + AttrNumber  num_phys_attrs;
> > +
> > + /*
> > +  * Pick up the required catalog information for each attribute in the
> > +  * relation, including the input function, the element type (to pass 
> > to
> > +  * the input function), and info about defaults and constraints. 
> > (Which
> > +  * input function we use depends on text/binary format choice.)
> > +  */
> > + num_phys_attrs = tupDesc->natts;
> > + in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
> > + typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));
>
> We need to update the comment because defaults and
> constraints aren't picked up here.
>
> > +CopyFromFormatTextStart(CopyFromState cstate, TupleDesc tupDesc)
> ...
> > + /*
> > +  * Pick up the required catalog information for each attribute in the
> > +  * relation, including the input function, the element type (to pass 
> > to
> > +  * the input function), and info about defaults and constraints. 
> > (Which
> > +  * input function we use depends on text/binary format choice.)
> > +  */
> > + in_functions = (FmgrInfo *) palloc(num_phys_attrs * sizeof(FmgrInfo));
> > + typioparams = (Oid *) palloc(num_phys_attrs * sizeof(Oid));
>
> ditto.
>
>
> > @@ -1716,15 +1776,6 @@ BeginCopyFrom(ParseState *pstate,
> >   ReceiveCopyBinaryHeader(cstate);
> >   }
>
> I think that this block should be moved to
> CopyFromFormatBinaryStart() too. But we need to run it after
> we setup inputs such as data_source_cb, pipe and filename...
>
> +/* Routines for a COPY HANDLER implementation. */
> +typedef struct CopyHandlerOps
> +{
> +   /* Called when COPY TO is started. This will send a header. */
> +   void(*copy_to_start) (CopyToState cstate, TupleDesc 
> tupDesc);
> +
> +   /* Copy one row for COPY TO. */
> +   void(*copy_to_one_row) (CopyToState cstate, 
> TupleTableSlot *slot);
> +
> +   /* Called when COPY TO is ended. This will send a trailer. */
> +   void(*copy_to_end) (CopyToState cstate);
> +
> +   void(*copy_from_start) (CopyFromState cstate, TupleDesc 
> tupDesc);
> +   bool(*copy_from_next) (CopyFromState cstate, ExprContext 
> *econtext,
> +  Datum 
> *values, bool *nulls);
> +   void(*copy_from_error_callback) (CopyFromState cstate);
> +   void(*copy_from_end) (CopyFromState cstate);
> +} CopyHandlerOps;
>
> It seems that "copy_" prefix is redundant. Should we use
> "to_start" instead of "copy_to_start" and so on?
>
> BTW, it seems that "COPY FROM (FORMAT json)" may not be implemented. [2]
> We may need to care about NULL copy_from_* cases.
>
>
> > I added a hook *copy_from_end* but this might be removed later if not used.
>
> It may be useful to clean up resources for COPY FROM but the
> patch doesn't call the copy_from_end. How about removing it
> for now? We can add it and call it from EndCopyFrom() later?
> Because it's not needed for now.
>
> I think that we should focus on refactoring instead of
> adding a new feature in this patch.
>
>
> [1]: 
> https://www.postgresql.org/message-id/20231204.153548.2126325458835528809.kou%40clear-code.com
> [2]: 
> https://www.postgresql.org/message-id/flat/CALvfUkBxTYy5uWPFVwpk_7ii2zgT07t3d-yR_cy4sfrrLU%3Dkcg%40mail.gmail.com
>
>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Thu, Dec 7, 2023 at 8:39 AM Michael Paquier  wrote:
>
> On Wed, Dec 06, 2023 at 10:07:51PM +0800, Junwang Zhao wrote:
> > I read the thread[1] you posted and I think Andres's suggestion sounds 
> > great.
> >
> > Should we extract both *copy to* and *copy from* for the first step, in that
> > case we can add the pg_copy_handler catalog smoothly later.
> >
> > Attached V4 adds 'extract copy from' and it passed the cirrus ci,
> > please take a look.
> >
> > I added a hook *copy_from_end* but this might be removed later if not used.
> >
> > [1]: 
> > https://www.postgresql.org/message-id/20180211211235.5x3jywe5z3lkgcsr%40alap3.anarazel.de
>
> I was looking at the differences between v3 posted by Sutou-san and
> v4 from you, seeing that:
>
> +/* Routines for a COPY HANDLER implementation. */
> +typedef struct CopyHandlerOps
>  {
>  /* Called when COPY TO is started. This will send a header. */
> -void(*start) (CopyToState cstate, TupleDesc tupDesc);
> +void(*copy_to_start) (CopyToState cstate, TupleDesc tupDesc);
>
>  /* Copy one row for COPY TO. */
> -void(*one_row) (CopyToState cstate, TupleTableSlot *slot);
> +void(*copy_to_one_row) (CopyToState cstate, TupleTableSlot 
> *slot);
>
>  /* Called when COPY TO is ended. This will send a trailer. */
> -void(*end) (CopyToState cstate);
> -} CopyToFormatOps;
> +void(*copy_to_end) (CopyToState cstate);
> +
> +void(*copy_from_start) (CopyFromState cstate, TupleDesc tupDesc);
> +bool(*copy_from_next) (CopyFromState cstate, ExprContext 
> *econtext,
> +Datum *values, bool *nulls);
> +void(*copy_from_error_callback) (CopyFromState cstate);
> +void(*copy_from_end) (CopyFromState cstate);
> +} CopyHandlerOps;
>
> And we've spent a good deal of time refactoring the copy code so as
> the logic behind TO and FROM is split.  Having a set of routines that
> groups both does not look like a step in the right direction to me,

The point of this refactor (from my view) is to make it possible to add new
copy handlers in extensions, just like access method. As Andres suggested,
a system catalog like *pg_copy_handler*, if we split TO and FROM into two
sets of routines, does that mean we have to create two catalog(
pg_copy_from_handler and pg_copy_to_handler)?

> and v4 is an attempt at solving two problems, while v3 aims to improve
> one case.  It seems to me that each callback portion should be focused
> on staying in its own area of the code, aka copyfrom*.c or copyto*.c.
> --
> Michael



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Wed, Dec 6, 2023 at 8:32 PM Daniel Verite  wrote:
>
> Sutou Kouhei wrote:
>
> > * 2022-04: Apache Arrow [2]
> > * 2018-02: Apache Avro, Apache Parquet and Apache ORC [3]
> >
> > (FYI: I want to add support for Apache Arrow.)
> >
> > There were discussions how to add support for more formats. [3][4]
> > In these discussions, we got a consensus about making COPY
> > format extendable.
>
>
> These formats seem all column-oriented whereas COPY is row-oriented
> at the protocol level [1].
> With regard to the procotol, how would it work to support these formats?
>

They have kind of *RowGroup* concepts, a bunch of rows goes to a RowBatch
and the data of the same column goes together.

I think they should fit the COPY semantics and there are some  FDW out there for
these modern formats, like [1]. If we support COPY to deal with the
format, it will
be easier to interact with them(without creating
server/usermapping/foreign table).

[1]: https://github.com/adjust/parquet_fdw

>
> [1] https://www.postgresql.org/docs/current/protocol-flow.html#PROTOCOL-COPY
>
>
> Best regards,
> --
> Daniel Vérité
> https://postgresql.verite.pro/
> Twitter: @DanielVerite
>
>


-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Wed, Dec 6, 2023 at 3:28 PM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Wed, 6 Dec 2023 15:11:34 +0800,
>   Junwang Zhao  wrote:
>
> > For the extra curly braces, I mean the following code block in
> > CopyToFormatBinaryStart:
> >
> > + {<-- I thought this is useless?
> > + /* Generate header for a binary copy */
> > + int32 tmp;
> > +
> > + /* Signature */
> > + CopySendData(cstate, BinarySignature, 11);
> > + /* Flags field */
> > + tmp = 0;
> > + CopySendInt32(cstate, tmp);
> > + /* No header extension */
> > + tmp = 0;
> > + CopySendInt32(cstate, tmp);
> > + }
>
> Oh, I see. I've removed and attach the v3 patch. In general,
> I don't change variable name and so on in this patch. I just
> move codes in this patch. But I also removed the "tmp"
> variable for this case because I think that the name isn't
> suitable for larger scope. (I think that "tmp" is acceptable
> in a small scope like the above code.)
>
> New code:
>
> /* Generate header for a binary copy */
> /* Signature */
> CopySendData(cstate, BinarySignature, 11);
> /* Flags field */
> CopySendInt32(cstate, 0);
> /* No header extension */
> CopySendInt32(cstate, 0);
>
>
> Thanks,
> --
> kou

Hi Kou,

I read the thread[1] you posted and I think Andres's suggestion sounds great.

Should we extract both *copy to* and *copy from* for the first step, in that
case we can add the pg_copy_handler catalog smoothly later.

Attached V4 adds 'extract copy from' and it passed the cirrus ci,
please take a look.

I added a hook *copy_from_end* but this might be removed later if not used.

[1]: 
https://www.postgresql.org/message-id/20180211211235.5x3jywe5z3lkgcsr%40alap3.anarazel.de
-- 
Regards
Junwang Zhao


v4-0001-Extract-COPY-handlers.patch
Description: Binary data

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Wed, Dec 6, 2023 at 2:19 PM Sutou Kouhei  wrote:
>
> Hi,
>
> In 
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Wed, 6 Dec 2023 11:18:35 +0800,
>   Junwang Zhao  wrote:
>
> > For the modern formats(parquet, orc, avro, etc.), will they be
> > implemented as extensions or in core?
>
> I think that they should be implemented as extensions
> because they will depend of external libraries and may not
> use C. For example, C++ will be used for Apache Parquet
> because the official Apache Parquet C++ implementation
> exists but the C implementation doesn't.
>
> (I can implement an extension for Apache Parquet after we
> complete this feature. I'll implement an extension for
> Apache Arrow with the official Apache Arrow C++
> implementation. And it's easy that we convert Apache Arrow
> data to Apache Parquet with the official Apache Parquet
> implementation.)
>
> > The patch looks good except for a pair of extra curly braces.
>
> Thanks for the review! I attach the v2 patch that removes
> extra curly braces for "if (isnull)".
>
For the extra curly braces, I mean the following code block in
CopyToFormatBinaryStart:

+ {<-- I thought this is useless?
+ /* Generate header for a binary copy */
+ int32 tmp;
+
+ /* Signature */
+ CopySendData(cstate, BinarySignature, 11);
+ /* Flags field */
+ tmp = 0;
+ CopySendInt32(cstate, tmp);
+ /* No header extension */
+ tmp = 0;
+ CopySendInt32(cstate, tmp);
+ }

>
> Thanks,
> --
> kou



-- 
Regards
Junwang Zhao

Re: Make COPY format extendable: Extract COPY TO format implementations

[Junwang Zhao]

On Wed, Dec 6, 2023 at 10:45 AM Sutou Kouhei  wrote:
>
> Hi,
>
> Thanks for replying to this proposal!
>
> In <20231205182458.GC2757816@nathanxps13>
>   "Re: Make COPY format extendable: Extract COPY TO format implementations" 
> on Tue, 5 Dec 2023 12:24:58 -0600,
>   Nathan Bossart  wrote:
>
> > I think it makes sense to do this part independently, but we should be
> > careful to design this with the follow-up tasks in mind.
>
> OK. I'll keep updating the "TODOs" section in the original
> e-mail. It also includes design in the follow-up tasks. We
> can discuss the design separately from the patches
> submitting. (The current submitted patch just focuses on
> refactoring but we can discuss the final design.)
>
> > I assume the performance concerns stem from the use of
> > function pointers.  Or was there something else?
>
> I think so too.
>
> The original e-mail that mentioned the performance concern
> [1] didn't say about the reason but the use of function
> pointers might be concerned.
>
> If the currently supported formats ("text", "csv" and
> "binary") are implemented as an extension, it may have more
> concerns but we will keep them as built-in formats for
> compatibility. So I think that no more concerns exist for
> these formats.
>

For the modern formats(parquet, orc, avro, etc.), will they be
implemented as extensions or in core?

The patch looks good except for a pair of extra curly braces.

>
> [1]: 
> https://www.postgresql.org/message-id/flat/3741749.1655952719%40sss.pgh.pa.us#2bb7af4a3d2c7669f9a49808d777a20d
>
>
> Thanks,
> --
> kou
>
>


-- 
Regards
Junwang Zhao

Re: Re: How to solve the problem of one backend process crashing and causing other processes to restart?

[Junwang Zhao]

On Mon, Nov 13, 2023 at 5:14 PM yuansong  wrote:
>
> Enhancing the overall fault tolerance of the entire system for this feature 
> is quite important. No one can avoid bugs, and I don't believe that this 
> approach is a more advanced one. It might be worth considering adding it to 
> the roadmap so that interested parties can conduct relevant research.
>
> The current major issue is that when one process crashes, resetting all 
> connections has a significant impact on other connections. Is it possible to 
> only disconnect the crashed connection and have the other connections simply 
> roll back the current transaction without reconnecting? Perhaps this problem 
> can be mitigated through the use of a connection pool.

It's not about the other connections, it's that the crashed connection
has no way to rollback.

>
> If we want to implement this feature, would it be sufficient to clean up or 
> restore the shared memory and disk changes caused by the crashed backend? 
> Besides clearing various known locks, what else needs to be changed? Does 
> anyone have any insights? Please help me. Thank you.
>
>
>
>
>
>
>
> At 2023-11-13 13:53:29, "Laurenz Albe"  wrote:
> >On Sun, 2023-11-12 at 21:55 -0500, Tom Lane wrote:
> >> yuansong  writes:
> >> > In PostgreSQL, when a backend process crashes, it can cause other backend
> >> > processes to also require a restart, primarily to ensure data 
> >> > consistency.
> >> > I understand that the correct approach is to analyze and identify the
> >> > cause of the crash and resolve it. However, it is also important to be
> >> > able to handle a backend process crash without affecting the operation of
> >> > other processes, thus minimizing the scope of negative impact and
> >> > improving availability. To achieve this goal, could we mimic the Oracle
> >> > process by introducing a "pmon" process dedicated to rolling back crashed
> >> > process transactions and performing resource cleanup? I wonder if anyone
> >> > has attempted such a strategy or if there have been previous discussions
> >> > on this topic.
> >>
> >> The reason we force a database-wide restart is that there's no way to
> >> be certain that the crashed process didn't corrupt anything in shared
> >> memory.  (Even with the forced restart, there's a window where bad
> >> data could reach disk before we kill off the other processes that
> >> might write it.  But at least it's a short window.)  "Corruption"
> >> here doesn't just involve bad data placed into disk buffers; more
> >> often it's things like unreleased locks, which would block other
> >> processes indefinitely.
> >>
> >> I seriously doubt that anything like what you're describing
> >> could be made reliable enough to be acceptable.  "Oracle does
> >> it like this" isn't a counter-argument: they have a much different
> >> (and non-extensible) architecture, and they also have an army of
> >> programmers to deal with minutiae like undoing resource acquisition.
> >> Even with that, you'd have to wonder about the number of bugs
> >> existing in such necessarily-poorly-tested code paths.
> >
> >Yes.
> >I think that PostgreSQL's approach is superior: rather than investing in
> >code to mitigate the impact of data corruption caused by a crash, invest
> >in quality code that doesn't crash in the first place.
> >
> >Euphemistically naming a crash "ORA-600 error" seems to be part of
> >their strategy.
> >
> >Yours,
> >Laurenz Albe
> >



-- 
Regards
Junwang Zhao

Re: make pg_ctl more friendly

[Junwang Zhao]

On Thu, Nov 9, 2023 at 3:08 PM Junwang Zhao  wrote:
>
> On Thu, Nov 9, 2023 at 9:57 AM Crisp Lee  wrote:
> >
> > Hi,
> >
> > I know it. But my question is not that. I did a PITR operation with 
> > recovery_target_name and recovery_target_action('shutdown'). The PITR 
> > process was very short and the PITR was done before pg_ctl check. The 
> > postmaster shutdown due to recovery_target_action, and there was no crash. 
> > But pg_ctl told me about startup failure.  I think the startup had 
> > succeeded and the result was not a exception. pg_ctl should tell users 
> > about detailed messages.
> >
> > On Thu, Nov 9, 2023 at 9:32 AM Andres Freund  wrote:
> >>
> >> Hi,
> >>
> >> On 2023-11-09 09:29:32 +0800, Crisp Lee wrote:
> >> > How to judge from 'DB_SHUTDOWNED' that PITR ends normally? 
> >> > 'DB_SHUTDOWNED'
> >> > is just a state, it could not give more meaning, so I reuse the
> >> > recovery.done.
> >>
> >> DB_SHUTDOWNED cannot be encountered while recovery is ongoing. If there 
> >> was a
> >> hard crash, you'd see DB_IN_ARCHIVE_RECOVERY or such, if the command was 
> >> shut
> >> down orderly before PITR has finished, you'd see DB_SHUTDOWNED_IN_RECOVERY.
> >>
> >> - Andres
>
> After a PITR shutdown, the db state should be *shut down in recovery*, try the
> patch attached.
>

previous patch has some format issues, V2 attached.

>
> --
> Regards
> Junwang Zhao



-- 
Regards
Junwang Zhao


v2-0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data

Re: make pg_ctl more friendly

[Junwang Zhao]

On Thu, Nov 9, 2023 at 9:57 AM Crisp Lee  wrote:
>
> Hi,
>
> I know it. But my question is not that. I did a PITR operation with 
> recovery_target_name and recovery_target_action('shutdown'). The PITR process 
> was very short and the PITR was done before pg_ctl check. The postmaster 
> shutdown due to recovery_target_action, and there was no crash. But pg_ctl 
> told me about startup failure.  I think the startup had succeeded and the 
> result was not a exception. pg_ctl should tell users about detailed messages.
>
> On Thu, Nov 9, 2023 at 9:32 AM Andres Freund  wrote:
>>
>> Hi,
>>
>> On 2023-11-09 09:29:32 +0800, Crisp Lee wrote:
>> > How to judge from 'DB_SHUTDOWNED' that PITR ends normally? 'DB_SHUTDOWNED'
>> > is just a state, it could not give more meaning, so I reuse the
>> > recovery.done.
>>
>> DB_SHUTDOWNED cannot be encountered while recovery is ongoing. If there was a
>> hard crash, you'd see DB_IN_ARCHIVE_RECOVERY or such, if the command was shut
>> down orderly before PITR has finished, you'd see DB_SHUTDOWNED_IN_RECOVERY.
>>
>> - Andres

After a PITR shutdown, the db state should be *shut down in recovery*, try the
patch attached.


-- 
Regards
Junwang Zhao


0001-PITR-shutdown-should-not-report-error-by-pg_ctl.patch
Description: Binary data

Re: Don't pass NULL pointer to strcmp().

[Junwang Zhao]

On Wed, Nov 1, 2023 at 5:25 PM Xing Guo  wrote:
>
> Hi hackers,
>
> I found that there's a nullable pointer being passed to strcmp() and
> can make the server crash. It can be reproduced on the latest master
> branch by crafting an extension[1]. Patch for fixing it is attatched.
>
> [1] https://github.com/higuoxing/guc_crash/tree/pg
>

Can we set a string guc to NULL? If not, `*lconf->variable == NULL` would
be unnecessary.

> --
> Best Regards,
> Xing



-- 
Regards
Junwang Zhao

Re: Simplify xlogreader.c with XLogRec* macros

[Junwang Zhao]

On Tue, Oct 31, 2023 at 5:23 PM 邱宇航  wrote:
>
> Hello hackers,
>
> Commit 3f1ce97 refactored XLog record access macros, but missed in a few 
> places. I fixed this, and patch is attached.
>
> --
> Yuhang Qiu
>
>
>

@@ -2036,8 +2035,8 @@ RestoreBlockImage(XLogReaderState *record, uint8
block_id, char *page)
  char*ptr;
  PGAlignedBlock tmp;

- if (block_id > record->record->max_block_id ||
- !record->record->blocks[block_id].in_use)
+ if (block_id > XLogRecMaxBlockId(record) ||
+ !XLogRecGetBlock(record, block_id)->in_use)

I thought these can also be rewrite to:

if (!XLogRecHasBlockRef(record, block_id))


-- 
Regards
Junwang Zhao

Re: Add trailing commas to enum definitions

[Junwang Zhao]

On Tue, Oct 24, 2023 at 4:34 AM Nathan Bossart  wrote:
>
> On Mon, Oct 23, 2023 at 05:55:32PM +0800, Junwang Zhao wrote:
> > On Mon, Oct 23, 2023 at 2:37 PM Peter Eisentraut  
> > wrote:
> >> Since C99, there can be a trailing comma after the last value in an enum
> >
> > C99 allows us to do this doesn't mean we must do this, this is not
> > inconsistent IMHO, and this will pollute the git log messages, people
> > may *git blame* the file and see the reason for the introduction of the
> > line.
>
> I suspect that your concerns about git-blame could be resolved by adding
> this commit to .git-blame-ignore-revs.  From a long-term perspective, I
> think standardizing on the trailing comma style will actually improve
> git-blame because patches won't need to add a comma to the previous line
> when adding a value.

make sense, +1 from me now.

>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com



-- 
Regards
Junwang Zhao

Re: Add trailing commas to enum definitions

[Junwang Zhao]

On Mon, Oct 23, 2023 at 2:37 PM Peter Eisentraut  wrote:
>
> Since C99, there can be a trailing comma after the last value in an enum

C99 allows us to do this doesn't mean we must do this, this is not
inconsistent IMHO, and this will pollute the git log messages, people
may *git blame* the file and see the reason for the introduction of the
line.

There are a lot of 'typedef struct' as well as 'struct', which is not
inconsistent either just like the *enum* case.

> definition.  A lot of new code has been introducing this style on the
> fly.  I have noticed that some new patches are now taking an
> inconsistent approach to this.  Some add the last comma on the fly if
> they add a new last value, some are trying to preserve the existing
> style in each place, some are even dropping the last comma if there was
> one.  I figured we could nudge this all in a consistent direction if we
> just add the trailing commas everywhere once.  See attached patch; it
> wasn't actually that much.
>
> I omitted a few places where there was a fixed "last" value that will
> always stay last.  I also skipped the header files of libpq and ecpg, in
> case people want to use those with older compilers.  There were also a
> small number of cases where the enum type wasn't used anywhere (but the
> enum values were), which ended up confusing pgindent a bit.



-- 
Regards
Junwang Zhao

Re: [dynahash] do not refill the hashkey after hash_search

[Junwang Zhao]

On Wed, Sep 13, 2023 at 5:28 PM John Naylor
 wrote:
>
>
> On Wed, Sep 13, 2023 at 3:46 PM Junwang Zhao  wrote:
> >
> > On Wed, Sep 13, 2023 at 4:22 PM John Naylor
> >  wrote:
>
> > > - memset(part_entry, 0, sizeof(LogicalRepPartMapEntry));
> > > - part_entry->partoid = partOid;
> > > + Assert(part_entry->partoid == partOid);
> > > + memset(entry, 0, sizeof(LogicalRepRelMapEntry));
> > >
> > > This is making an assumption that the non-key part of 
> > > LogicalRepPartMapEntry will never get new members. Without knowing much 
> > > about this code, it seems like a risk in the abstract.
> >
> > What do you mean by 'the non-key part of LogicalRepPartMapEntry will
> > never get new members'?
>
> I mean, if this struct:
>
> > typedef struct LogicalRepPartMapEntry
> > {
> > Oid partoid; /* LogicalRepPartMap's key */
> > LogicalRepRelMapEntry relmapentry;
> > } LogicalRepPartMapEntry;
>
> ...gets a new member, it will not get memset when memsetting "relmapentry".

ok, I see. I will leave this case as it was.

>
> > > Taking a quick look, I didn't happen to see any existing asserts of this 
> > > sort, so the patch doesn't seem to be making things more "normal". I did 
> > > see a few instances of /* hash_search already filled in the key */, so if 
> > > we do anything at all here, we might prefer that.
> >
> > There are some code using assert for this sort, for example in
> > *ReorderBufferToastAppendChunk*:
>
> > and in *rebuild_database_list*, tom commented that the key has already
> > been filled, which I think
> > he was trying to tell people no need to assign the key again.
>
> Okay, we have examples of each.
>
> --
> John Naylor
> EDB: http://www.enterprisedb.com

Add a v2 with some change to fix warnings about unused-parameter.

I will add this to Commit Fest.

-- 
Regards
Junwang Zhao


v2-0001-do-not-refill-the-hashkey-after-hash_search.patch
Description: Binary data

Re: [dynahash] do not refill the hashkey after hash_search

[Junwang Zhao]

On Wed, Sep 13, 2023 at 4:22 PM John Naylor
 wrote:
>
>
> On Wed, Sep 13, 2023 at 1:47 PM Junwang Zhao  wrote:
> >
> > Hi hackers,
> >
> > It's not necessary to fill the key field for most cases, since
> > hash_search has already done that for you. For developer that
> > using memset to zero the entry structure after enter it, fill the
> > key field is a must, but IMHO that is not good coding style, we
> > really should not touch the key field after insert it into the
> > dynahash.
>
> - memset(part_entry, 0, sizeof(LogicalRepPartMapEntry));
> - part_entry->partoid = partOid;
> + Assert(part_entry->partoid == partOid);
> + memset(entry, 0, sizeof(LogicalRepRelMapEntry));
>
> This is making an assumption that the non-key part of LogicalRepPartMapEntry 
> will never get new members. Without knowing much about this code, it seems 
> like a risk in the abstract.

What do you mean by 'the non-key part of LogicalRepPartMapEntry will
never get new members'?

typedef struct LogicalRepPartMapEntry
{
Oid partoid; /* LogicalRepPartMap's key */
LogicalRepRelMapEntry relmapentry;
} LogicalRepPartMapEntry;

partoid has already been filled by hash_search with HASH_ENTER action,
so I think the
above code should have the same effects.

>
> > This patch fixed some most abnormal ones, instead of refilling the
> > key field of primitive types, adding some assert might be a better
> > choice.
>
> Taking a quick look, I didn't happen to see any existing asserts of this 
> sort, so the patch doesn't seem to be making things more "normal". I did see 
> a few instances of /* hash_search already filled in the key */, so if we do 
> anything at all here, we might prefer that.

There are some code using assert for this sort, for example in
*ReorderBufferToastAppendChunk*:

```
ent = (ReorderBufferToastEnt *)
hash_search(txn->toast_hash, _id, HASH_ENTER, );

if (!found)
{
Assert(ent->chunk_id == chunk_id);   <--- this
line, by Robert Haas
ent->num_chunks = 0;
ent->last_chunk_seq = 0;
```

and in *rebuild_database_list*, tom commented that the key has already
been filled, which I think
he was trying to tell people no need to assign the key again.

```
/* we assume it isn't found because the hash was just created */
db = hash_search(dbhash, , HASH_ENTER, NULL);

/* hash_search already filled in the key */ <--- this
line, by Tom Lane
db->adl_score = score++;
/* next_worker is filled in later */
```

>
> - hash_search(uncommitted_enums, serialized++, HASH_ENTER, NULL);
> + (void) hash_search(uncommitted_enums, serialized++, HASH_ENTER, NULL);
>
> I prefer explicit (void) for new code, but others may disagree. I don't think 
> we have a preferred style for this, so changing current usage will just cause 
> unnecessary code churn.
>

What I am concerned about is that if we change the key after
hash_search with HASH_ENTER action, there
are chances that if we assign a wrong value, it will be impossible to
match that entry again.

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



-- 
Regards
Junwang Zhao

[dynahash] do not refill the hashkey after hash_search

[Junwang Zhao]

Hi hackers,

It's not necessary to fill the key field for most cases, since
hash_search has already done that for you. For developer that
using memset to zero the entry structure after enter it, fill the
key field is a must, but IMHO that is not good coding style, we
really should not touch the key field after insert it into the
dynahash.

This patch fixed some most abnormal ones, instead of refilling the
key field of primitive types, adding some assert might be a better
choice.


-- 
Regards
Junwang Zhao


0001-do-not-refill-the-hashkey-after-hash_search.patch
Description: Binary data

Re: [BackendXidGetPid] only access allProcs when xid matches

[Junwang Zhao]

On Thu, Sep 7, 2023 at 5:07 PM Ashutosh Bapat
 wrote:
>
> Forgot to attach the patch.

LGTM

Should I change the status to ready for committer now?

>
> On Thu, Sep 7, 2023 at 1:22 PM Ashutosh Bapat
>  wrote:
> >
> > Hi Junwang,
> > We leave a line blank after variable declaration as in the attached patch.
> >
> > Otherwise the patch looks good to me.
> >
> > The function modified by the patch is only used by extension
> > pgrowlocks. Given that the function will be invoked as many times as
> > the number of locked rows in the relation, the patch may show some
> > improvement and thus be more compelling. One way to measure
> > performance is to create a table with millions of rows, SELECT all
> > rows with FOR SHARE/UPDATE clause. Then run pgrowlock() on that
> > relation. This will invoke the given function a million times. That
> > way we might be able to catch some miniscule improvement per row.
> >
> > If the performance is measurable, we can mark the CF entry as ready
> > for committer.
> >
> > --
> > Best Wishes,
> > Ashutosh Bapat
> >
> > On Thu, Aug 10, 2023 at 1:48 PM Junwang Zhao  wrote:
> > >
> > > On Thu, Aug 10, 2023 at 4:11 PM Ashutosh Bapat
> > >  wrote:
> > > >
> > > > Please add this to commitfest so that it's not forgotten.
> > > >
> > >
> > > Added [1], thanks
> > >
> > > [1]: https://commitfest.postgresql.org/44/4495/
> > >
> > > > On Wed, Aug 9, 2023 at 8:37 PM Junwang Zhao  wrote:
> > > > >
> > > > > On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
> > > > >  wrote:
> > > > > >
> > > > > > On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao  
> > > > > > wrote:
> > > > > > >
> > > > > > > In function `BackendXidGetPid`, when looping every proc's
> > > > > > > TransactionId, there is no need to access its PGPROC since 
> > > > > > > there
> > > > > > > is shared memory access: `arrayP->pgprocnos[index]`.
> > > > > > >
> > > > > > > Though the compiler can optimize this kind of inefficiency, I
> > > > > > > believe we should ship with better code.
> > > > > > >
> > > > > >
> > > > > > Looks good to me. However, I would just move the variable 
> > > > > > declaration
> > > > > > with their assignments inside the if () rather than combing the
> > > > > > expressions. It more readable that way.
> > > > >
> > > > > yeah, make sense, also checked elsewhere using the original style,
> > > > > attachment file
> > > > > keep that style, thanks ;)
> > > > >
> > > > > >
> > > > > > --
> > > > > > Best Wishes,
> > > > > > Ashutosh Bapat
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Regards
> > > > > Junwang Zhao
> > > >
> > > >
> > > >
> > > > --
> > > > Best Wishes,
> > > > Ashutosh Bapat
> > >
> > >
> > >
> > > --
> > > Regards
> > > Junwang Zhao
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat



-- 
Regards
Junwang Zhao

Re: [BackendXidGetPid] only access allProcs when xid matches

[Junwang Zhao]

On Thu, Sep 7, 2023 at 4:04 PM Michael Paquier  wrote:
>
> On Thu, Sep 07, 2023 at 01:22:07PM +0530, Ashutosh Bapat wrote:
> > Otherwise the patch looks good to me.
> >
> > The function modified by the patch is only used by extension
> > pgrowlocks. Given that the function will be invoked as many times as
> > the number of locked rows in the relation, the patch may show some
> > improvement and thus be more compelling. One way to measure
> > performance is to create a table with millions of rows, SELECT all
> > rows with FOR SHARE/UPDATE clause. Then run pgrowlock() on that
> > relation. This will invoke the given function a million times. That
> > way we might be able to catch some miniscule improvement per row.
> >
> > If the performance is measurable, we can mark the CF entry as ready
> > for committer.
>
> So, is the difference measurable?  Assuming that your compiler does
I have checked my compiler, this patch give them same assembly code
as before.
> not optimize that, my guess is no because the cycles are going to be
> eaten by the other system calls in pgrowlocks.  You could increase the
> number of proc entries and force a large loop around
> BackendXidGetPid() to see a difference of runtime, but that's going
> to require a lot more proc entries to see any kind of difference
> outside the scope of a usual ~1% (somewhat magic number!) noise with
> such micro benchmarks.
>
> -intpgprocno = arrayP->pgprocnos[index];
> -PGPROC   *proc = [pgprocno];
> -
>  if (other_xids[index] == xid)
>  {
> +PGPROC   *proc = [arrayP->pgprocnos[index]];
>  result = proc->pid;
>  break;
>
> Saying that, moving the declarations into the inner loop is usually a
> good practice, but I would just keep two variables instead of one for
> the sake of readability.  That's a nit, sure.

I remember I split this into two lines in v2 patch. Whatever, I attached
a v3 with a suggestion from Ashutosh Bapat.

> --
> Michael



-- 
Regards
Junwang Zhao


v3-0001-BackendXidGetPid-only-access-allProcs-when-xid-ma.patch
Description: Binary data

Re: Should we use MemSet or {0} for struct initialization?

[Junwang Zhao]

On Thu, Aug 31, 2023 at 7:07 PM John Naylor
 wrote:
>
> > On Thu, Aug 31, 2023 at 5:34 PM Richard Guo  wrote:
> > >
> > > While working on a bug in expandRecordVariable() I noticed that in the
> > > switch statement for case RTE_SUBQUERY we initialize struct ParseState
> > > with {0} while for case RTE_CTE we do that with MemSet.  I understand
> > > that there is nothing wrong with this, just cannot get away with the
> > > inconsistency inside the same function (sorry for the nitpicking).
> > >
> > > Do we have a preference for how to initialize structures?  From 9fd45870
> > > it seems that we prefer to {0}.  So here is a trivial patch doing that.
>
> It seems to have been deliberately left that way in the wake of that commit, 
> see:
>
> https://www.postgresql.org/message-id/87d2e5f8-3c37-d185-4bbc-1de163ac4b10%40enterprisedb.com
>
> (If so, it deserves a comment to keep people from trying to change it...)
>
> > > And with a rough scan the MemSet calls in pg_stat_get_backend_subxact()
> > > can also be replaced with {0}, so include that in the patch too.
>
> I _believe_ that's harmless to change.
>
> On Thu, Aug 31, 2023 at 4:57 PM Junwang Zhao  wrote:
>
> > If the struct has padding or aligned, {0} only guarantee the struct
> > members initialized to 0, while memset sets the alignment/padding
> > to 0 as well, but since we will not access the alignment/padding, so
> > they give the same effect.
>
> See above -- if it's used as a hash key, for example, you must clear 
> everything.

Yeah, if memcmp was used as the key comparison function, there is a problem.

>
> > I bet {0} should be faster since there is no function call, but I'm not
> > 100% sure ;)
>
> Neither has a function call. MemSet is a PG macro. You're thinking of memset, 
> the libc library function, but a decent compiler can easily turn that into 
> something else for fixed-size inputs.

good to know, thanks.

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


-- 
Regards
Junwang Zhao

Re: Should we use MemSet or {0} for struct initialization?

[Junwang Zhao]

On Thu, Aug 31, 2023 at 5:34 PM Richard Guo  wrote:
>
> While working on a bug in expandRecordVariable() I noticed that in the
> switch statement for case RTE_SUBQUERY we initialize struct ParseState
> with {0} while for case RTE_CTE we do that with MemSet.  I understand
> that there is nothing wrong with this, just cannot get away with the
> inconsistency inside the same function (sorry for the nitpicking).
>
> Do we have a preference for how to initialize structures?  From 9fd45870
> it seems that we prefer to {0}.  So here is a trivial patch doing that.
> And with a rough scan the MemSet calls in pg_stat_get_backend_subxact()
> can also be replaced with {0}, so include that in the patch too.
>
> Thanks
> Richard

If the struct has padding or aligned, {0} only guarantee the struct
members initialized to 0, while memset sets the alignment/padding
to 0 as well, but since we will not access the alignment/padding, so
they give the same effect.

I bet {0} should be faster since there is no function call, but I'm not
100% sure ;)


-- 
Regards
Junwang Zhao

Re: [PATCH v1] PQputCopyEnd never returns 0, fix the inaccurate comment

[Junwang Zhao]

On Tue, Aug 29, 2023 at 6:40 AM Michael Paquier  wrote:
>
> On Mon, Aug 28, 2023 at 09:46:07PM +0800, Junwang Zhao wrote:
> > Yeah, it makes sense to me, or maybe just `PQputCopyEnd(...) == -1`,
> > let's wait for some other opinions.
>
> One can argue that PQputCopyEnd() returning 0 could be possible in an
> older version of libpq these callers are linking to, but this has
> never existed from what I can see (just checked down to 8.2 now).
> Anyway, changing these callers may create some backpatching conflicts,
> so I'd let them as they are, and just fix the comment.

sure, thanks for the explanation.

> --
> Michael



-- 
Regards
Junwang Zhao

Re: [PATCH v1] PQputCopyEnd never returns 0, fix the inaccurate comment

[Junwang Zhao]

On Mon, Aug 28, 2023 at 7:48 PM Aleksander Alekseev
 wrote:
>
> Hi Junwang,
>
> > PQputCopyEnd returns 1 or -1, never 0, I guess the comment was
> > copy/paste from PQputCopyData's comment, this should be fixed.
>
> The patch LGTM but I wonder whether we should also change all the
> existing calls of PQputCopyEnd() from:
>
> ```
> PQputCopyEnd(...) <= 0
> ```
>
> ... to:
>
> ```
> PQputCopyEnd(...) < 0
> ```
>
> Given the circumstances, checking for equality to zero seems to be at
> least strange.
>

Yeah, it makes sense to me, or maybe just `PQputCopyEnd(...) == -1`,
let's wait for some other opinions.

>
> On top of that, none of the PQputCopyData() callers cares whether the
> function returns 0 or -1, both are treated the same way. I suspect the
> function does some extra work no one asked to do and no one cares
> about. Perhaps this function should be refactored too for consistency.
>
> Thoughts?
>
> --
> Best regards,
> Aleksander Alekseev



-- 
Regards
Junwang Zhao

[PATCH v1] PQputCopyEnd never returns 0, fix the inaccurate comment

[Junwang Zhao]

PQputCopyEnd returns 1 or -1, never 0, I guess the comment was
copy/paste from PQputCopyData's comment, this should be fixed.

-- 
Regards
Junwang Zhao


v1-0001-PQputCopyEnd-never-returns-0-fix-the-inaccurate-c.patch
Description: Binary data

Re: [dsm] comment typo

[Junwang Zhao]

On Mon, Aug 21, 2023 at 5:16 PM Daniel Gustafsson  wrote:
>
> > On 18 Aug 2023, at 11:10, Junwang Zhao  wrote:
> >
> > In the following sentence, I believe either 'the' or 'a' should be kept, not
> > both. I here keep the 'the', but feel free to change.
>
> >  * handle: The handle of an existing object, or for DSM_OP_CREATE, the
> > - *a new handle the caller wants created.
> > + *new handle the caller wants created.
>
> Since the handle doesn't exist for DSM_OP_CREATE, both "a handle" and "the
> handle" seems a tad misleading, how about "the identifier for the new handle 
> the
> caller wants created"?
>

Sounds great 

> --
> Daniel Gustafsson
>


-- 
Regards
Junwang Zhao

[dsm] comment typo

[Junwang Zhao]

In the following sentence, I believe either 'the' or 'a' should be kept, not
both. I here keep the 'the', but feel free to change.

---
 src/backend/storage/ipc/dsm_impl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/storage/ipc/dsm_impl.c
b/src/backend/storage/ipc/dsm_impl.c
index 6399fa2ad5..19a9cfc8ac 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -137,7 +137,7 @@ int min_dynamic_shared_memory;
  * Arguments:
  * op: The operation to be performed.
  * handle: The handle of an existing object, or for DSM_OP_CREATE, the
- *a new handle the caller wants created.
+ *new handle the caller wants created.
  * request_size: For DSM_OP_CREATE, the requested size.  Otherwise, 0.
  * impl_private: Private, implementation-specific data.  Will be a pointer
  *to NULL for the first operation on a shared memory segment within this
-- 

-- 
Regards
Junwang Zhao

[question] difference between vm_extend and fsm_extend

[Junwang Zhao]

Hey hacks,

I'm looking the code of free space map and visibility map, both the module
call  `ExtendBufferedRelTo` to extend its file when necessary, what confused
me is that `vm_extend` has an extra step that send a shared message to force
other backends to close other smgr references.

My question is why does `fsm_extend` not need the invalidation step?

-- 
Regards
Junwang Zhao

Re: [BackendXidGetPid] only access allProcs when xid matches

[Junwang Zhao]

On Thu, Aug 10, 2023 at 4:11 PM Ashutosh Bapat
 wrote:
>
> Please add this to commitfest so that it's not forgotten.
>

Added [1], thanks

[1]: https://commitfest.postgresql.org/44/4495/

> On Wed, Aug 9, 2023 at 8:37 PM Junwang Zhao  wrote:
> >
> > On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
> >  wrote:
> > >
> > > On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao  wrote:
> > > >
> > > > In function `BackendXidGetPid`, when looping every proc's
> > > > TransactionId, there is no need to access its PGPROC since there
> > > > is shared memory access: `arrayP->pgprocnos[index]`.
> > > >
> > > > Though the compiler can optimize this kind of inefficiency, I
> > > > believe we should ship with better code.
> > > >
> > >
> > > Looks good to me. However, I would just move the variable declaration
> > > with their assignments inside the if () rather than combing the
> > > expressions. It more readable that way.
> >
> > yeah, make sense, also checked elsewhere using the original style,
> > attachment file
> > keep that style, thanks ;)
> >
> > >
> > > --
> > > Best Wishes,
> > > Ashutosh Bapat
> >
> >
> >
> > --
> > Regards
> > Junwang Zhao
>
>
>
> --
> Best Wishes,
> Ashutosh Bapat



-- 
Regards
Junwang Zhao

Re: [BackendXidGetPid] only access allProcs when xid matches

[Junwang Zhao]

On Wed, Aug 9, 2023 at 10:46 PM Ashutosh Bapat
 wrote:
>
> On Wed, Aug 9, 2023 at 9:30 AM Junwang Zhao  wrote:
> >
> > In function `BackendXidGetPid`, when looping every proc's
> > TransactionId, there is no need to access its PGPROC since there
> > is shared memory access: `arrayP->pgprocnos[index]`.
> >
> > Though the compiler can optimize this kind of inefficiency, I
> > believe we should ship with better code.
> >
>
> Looks good to me. However, I would just move the variable declaration
> with their assignments inside the if () rather than combing the
> expressions. It more readable that way.

yeah, make sense, also checked elsewhere using the original style,
attachment file
keep that style, thanks ;)

>
> --
> Best Wishes,
> Ashutosh Bapat



-- 
Regards
Junwang Zhao


v2-0001-BackendXidGetPid-only-access-allProcs-when-xid-ma.patch
Description: Binary data

[BackendXidGetPid] only access allProcs when xid matches

[Junwang Zhao]

In function `BackendXidGetPid`, when looping every proc's
TransactionId, there is no need to access its PGPROC since there
is shared memory access: `arrayP->pgprocnos[index]`.

Though the compiler can optimize this kind of inefficiency, I
believe we should ship with better code.


-- 
Regards
Junwang Zhao


0001-BackendXidGetPid-only-access-allProcs-when-xid-match.patch
Description: Binary data

[PATCH] [zh_CN.po] fix a typo in simplified Chinese translation file

[Junwang Zhao]

add the missing leading `l` for log_statement_sample_rate

-- 
Regards
Junwang Zhao


0001-zh_CN.po-fix-a-typo-in-simplified-Chinese-translatio.patch
Description: Binary data

Re: pg_upgrade fails with in-place tablespace

[Junwang Zhao]

On Mon, Jul 31, 2023 at 5:36 PM Rui Zhao  wrote:
>
> Hello postgres hackers,
> Recently I encountered an issue: pg_upgrade fails when dealing with in-place 
> tablespace. As we know, pg_upgrade uses pg_dumpall to dump objects and 
> pg_restore to restore them. The problem seems to be that pg_dumpall is 
> dumping in-place tablespace as relative path, which can't be restored.
>
> Here is the error message of pg_upgrade:
> psql:/home/postgres/postgresql/src/bin/pg_upgrade/tmp_check/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20230729T210058.329/dump/pg_upgrade_dump_globals.sql:36:
>  ERROR:  tablespace location must be an absolute path
>
> To help reproduce the failure, I have attached a tap test. The test also 
> fails with tablespace regression, and it change the default value of 
> allow_in_place_tablespaces to true only for debug, so it may not be fit for 
> production. However, it is enough to reproduce this failure.
> I have also identified a solution for this problem, which I have included in 
> the patch. The solution has two modifications:
>   1) Make the function pg_tablespace_location returns path "" with in-place 
> tablespace, rather than relative path. Because the path of the in-place 
> tablespace is always 'pg_tblspc/'.
>   2) Only check the tablespace with an absolute path in pg_upgrade.
>
>   There are also other solutions, such as supporting the creation of 
> relative-path tablespace in function CreateTableSpace. But do we really need 
> relative-path tablespace? I think in-place tablespace is enough by now. My 
> solution may be more lightweight and harmless.
>
> Thank you for your attention to this matter.
>
> Best regards,
> Rui Zhao

Seems like allow_in_place_tablespaces is a developer only guc, and it
is not for end user usage.
check this commit 7170f2159fb21b62c263acd458d781e2f3c3f8bb


-- 
Regards
Junwang Zhao

Re: table_open/table_close with different lock mode

[Junwang Zhao]

On Fri, Jul 21, 2023 at 2:57 PM Gurjeet Singh  wrote:
>
> On Thu, Jul 20, 2023 at 11:38 PM Junwang Zhao  wrote:
> >
> > On Fri, Jul 21, 2023 at 2:26 PM Michael Paquier  wrote:
> > >
> > > On Fri, Jul 21, 2023 at 02:05:56PM +0800, Junwang Zhao wrote:
> > > > I noticed there are some places calling table_open with
> > > > RowExclusiveLock but table_close with NoLock, like in function
> > > > toast_save_datum.
> > > >
> > > > Can anybody explain the underlying logic, thanks in advance.
> > >
> > > This rings a bell.  This is a wanted behavior, see commit f99870d and
> > > its related thread:
> > > https://postgr.es/m/17268-d2fb426e0895a...@postgresql.org
> > >
> >
> > I see this patch, so all the locks held by a transaction will be released
> > at the commit phase, right? Can you show me where the logic is located?
>
> The NoLock is simple a marker that tells the underlying machinery to
> not bother releasing any locks. As a matter of fact, you can pass
> NoLock in *_open() calls, too, to indicate that you don't want any new
> locks, perhaps because the transaction has already taken an
> appropriate lock on the object.
>
> As for lock-releasing codepath at transaction end, see
> CommitTransaction() in xact.c, and specifically at the
> ResourceOwnerRelease() calls in there.
>

Great, thanks for the thorough explanation, will look into the code :)

> Best regards,
> Gurjeet
> http://Gurje.et



-- 
Regards
Junwang Zhao

Re: table_open/table_close with different lock mode

[Junwang Zhao]

On Fri, Jul 21, 2023 at 2:26 PM Michael Paquier  wrote:
>
> On Fri, Jul 21, 2023 at 02:05:56PM +0800, Junwang Zhao wrote:
> > I noticed there are some places calling table_open with
> > RowExclusiveLock but table_close with NoLock, like in function
> > toast_save_datum.
> >
> > Can anybody explain the underlying logic, thanks in advance.
>
> This rings a bell.  This is a wanted behavior, see commit f99870d and
> its related thread:
> https://postgr.es/m/17268-d2fb426e0895a...@postgresql.org
>

I see this patch, so all the locks held by a transaction will be released
at the commit phase, right? Can you show me where the logic is located?

> The tests added by this commit in src/test/isolation/ will show the
> difference in terms of the way the toast values get handled with and
> without the change.
> --
> Michael



-- 
Regards
Junwang Zhao

table_open/table_close with different lock mode

[Junwang Zhao]

Hey hackers,

I noticed there are some places calling table_open with
RowExclusiveLock but table_close with NoLock, like in function
toast_save_datum.

Can anybody explain the underlying logic, thanks in advance.

-- 
Regards
Junwang Zhao