pgbench with partitioned tables
Hello, hackers!
When using manually created partitioned tables for pgbench, an error
occurred:
ERROR: cannot perform COPY FREEZE on a partitioned table.
Currently only pgbench_accounts can be partitioned, all others must be a
regular tables.
I wrote a patch to disable WITH (FREEZE = ON) when generating data for
non-regular tables.
See attached patch
--
With best regards,
Sergey Tatarintsev
Attachments:
v1-0001-Fix-pgbench-client-side-data-generation-f.patchtext/x-patch; charset=UTF-8; name=v1-0001-Fix-pgbench-client-side-data-generation-f.patchDownload
From 9aaf2b222d6eb0c218c8a417280a8fdc0a42c296 Mon Sep 17 00:00:00 2001
From: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Date: Thu, 30 Jan 2025 14:52:29 +0700
Subject: [PATCH-v1] Fix pgbench client-side data generation for partitioned tables
Client-side data generation cannot perform COPY WITH (FREEZE = ON) on partitioned
tables except pgbench_accounts.
Patch disables FREEZE for any non-regular tables
---
src/bin/pgbench/pgbench.c | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b5cf3c6ed01..cac5312c4b1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -847,6 +847,28 @@ static const PsqlScanCallbacks pgbench_callbacks = {
NULL, /* don't need get_variable functionality */
};
+static bool
+is_regular_table(PGconn *con, const char *table)
+{
+ PGresult *res;
+ char *sql = psprintf("SELECT relkind FROM pg_class WHERE oid='%s'::regclass::oid", table);
+ bool is_regular_table = true;
+
+ res = PQexec(con, sql);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ pg_log_error("query failed: %s", PQerrorMessage(con));
+ pg_log_error_detail("Query was: %s", sql);
+ exit(1);
+ }
+ if (!PQgetisnull(res, 0, 0))
+ is_regular_table = strncmp(PQgetvalue(res, 0, 0), "r", 1) == 0;
+
+ PQclear(res);
+ pfree(sql);
+ return is_regular_table;
+}
+
static inline pg_time_usec_t
pg_time_now(void)
{
@@ -4958,16 +4980,10 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
initPQExpBuffer(&sql);
- /*
- * Use COPY with FREEZE on v14 and later for all the tables except
- * pgbench_accounts when it is partitioned.
- */
- if (PQserverVersion(con) >= 140000)
- {
- if (strcmp(table, "pgbench_accounts") != 0 ||
- partitions == 0)
+ /* Use COPY with FREEZE on v14 and later for all regular tables */
+ if ((PQserverVersion(con) >= 140000) && is_regular_table(con, table))
copy_statement_fmt = "copy %s from stdin with (freeze on)";
- }
+
n = pg_snprintf(copy_statement, sizeof(copy_statement), copy_statement_fmt, table);
if (n >= sizeof(copy_statement))
--
2.43.0
pgbench_accounts is the only table that should grow to
the point where partitioning can benefit the tpcb-like
benchmark.
It looks like you have customized the pgbench
schema that partitions the other pgbench tables, so
you can use a custom script to initialize the data.
IMO, If there is a good reason to allow the other pgbench
tables to be partitioned, that may be better to think
about. I am not sure there is though.
Regards,
Sami
Amazon Web Services (AWS)
On Fri, Jan 31, 2025 at 2:27 PM Sami Imseih <samimseih@gmail.com> wrote:
pgbench_accounts is the only table that should grow to
the point where partitioning can benefit the tpcb-like
benchmark.
pgbench_history is append-only, so I could see the argument for
partitioning that.
IMO, If there is a good reason to allow the other pgbench
tables to be partitioned, that may be better to think
about. I am not sure there is though.
see this thread [1]/messages/by-id/CAAKRu_Zo8ST-Qk8VQ4KFkbMQcqJsQQz5r+YRRbecS3avgkoZhw@mail.gmail.com proposing partitioning pgbench_history last year.
[1]: /messages/by-id/CAAKRu_Zo8ST-Qk8VQ4KFkbMQcqJsQQz5r+YRRbecS3avgkoZhw@mail.gmail.com
On Fri, Jan 31, 2025 at 6:32 AM Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
When using manually created partitioned tables for pgbench, an error
occurred:
ERROR: cannot perform COPY FREEZE on a partitioned table.Currently only pgbench_accounts can be partitioned, all others must be a
regular tables.I wrote a patch to disable WITH (FREEZE = ON) when generating data for
non-regular tables.
Thanks for the patch!
I recommend including a repro when proposing such things. Here's one I made
# pgbench init so all the tables are there
pgbench -i -s 10
# drop pgbench_tellers and recreate it partitioned
psql -f- <<EOF
DROP TABLE IF EXISTS pgbench_tellers;
CREATE TABLE pgbench_tellers (
tid INT NOT NULL,
bid INT,
tbalance INT,
filler CHAR(84)
) PARTITION BY RANGE (tid);
CREATE TABLE pgbench_tellers_p1
PARTITION OF pgbench_tellers
FOR VALUES FROM (1) TO (50);
CREATE TABLE pgbench_tellers_p2
PARTITION OF pgbench_tellers
FOR VALUES FROM (50) TO (2000);
EOF
# run pgbench init with only the data gen step
pgbench -i -I g -s 10
Personally, I have needed to disable COPY FREEZE during pgbench -i
when benchmarking features related to vacuum's freezing behavior.
Maybe instead of just not using COPY FREEZE on a table if it is
partitioned, we could add new data generation init_steps. Perhaps one
that is client-side data generation (g) but with no freezing? I'm not
really sure what the letter would be (f? making it f, g, and G?).
It seems they did not consider making COPY FREEZE optional when adding
the feature to pgbench [1]/messages/by-id/20210308.143907.2014279678657453983.t-ishii@gmail.com.
This differs from your patch's behavior but it might still solve your problem?
- Melanie
[1]: /messages/by-id/20210308.143907.2014279678657453983.t-ishii@gmail.com
IMO, If there is a good reason to allow the other pgbench
tables to be partitioned, that may be better to think
about. I am not sure there is though.see this thread [1] proposing partitioning pgbench_history last year.
[1] /messages/by-id/CAAKRu_Zo8ST-Qk8VQ4KFkbMQcqJsQQz5r+YRRbecS3avgkoZhw@mail.gmail.com
I don't see the partitioning history being beneficial for the
built-in workloads, but it may make sense to partition the
history table if you intend to run a custom benchmark that
includes reading specific accounts from the history table.
Partitioning by date range as you suggest [1]/messages/by-id/CAAKRu_Zo8ST-Qk8VQ4KFkbMQcqJsQQz5r+YRRbecS3avgkoZhw@mail.gmail.com makes sense as well.
Maybe It will be good to provide more flexibility around which
tables to partition and the partition key(s) for the pgbench schema so
to benchmark different partition strategies.
[1]: /messages/by-id/CAAKRu_Zo8ST-Qk8VQ4KFkbMQcqJsQQz5r+YRRbecS3avgkoZhw@mail.gmail.com
Regards,
Sami
On 2025-Jan-31, Melanie Plageman wrote:
Maybe instead of just not using COPY FREEZE on a table if it is
partitioned, we could add new data generation init_steps. Perhaps one
that is client-side data generation (g) but with no freezing? I'm not
really sure what the letter would be (f? making it f, g, and G?).
I think it makes sense to do what you suggest, but on the other hand,
the original code that Sergey is patching looks like a hack in the sense
that it hardcodes which tables to use FREEZE with. There's no point to
doing things that way actually, so accepting Sergey's patch to replace
that with a relkind check feels sensible to me.
I think the query should be
SELECT relkind FROM pg_catalog.pg_class WHERE oid='%s'::pg_catalog.regclass
if only for consistency with pgbench's other query on catalogs.
Your proposal to add different init_steps might be reasonable, at least
if we allowed partitionedness of tables to vary in other ways (eg. if we
made pgbench_history partitioned), but I don't think it conflicts with
Sergey's patch in spirit.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Here's a general engineering tip: if the non-fun part is too complex for you
to figure out, that might indicate the fun part is too ambitious." (John Naylor)
/messages/by-id/CAFBsxsG4OWHBbSDM=sSeXrQGOtkPiOEOuME4yD7Ce41NtaAD9g@mail.gmail.com
I was looking at the comments [1]https://github.com/postgres/postgres/blob/master/src/backend/commands/copyfrom.c#L727-L735 for why COPY FREEZE is
not allowed on a parent table, and it was mainly due
to having to open up partitions to check if they are able
to take the optimization (table created or truncated in the
current transaction ). Obviously as the number of
partitions grow, it will take more to perform these
checks. My suspicious is that in the cases in which partitions
are used, the benefits of COPY FREEZE could outweigh
the overhead of these additional checks.
So while we could try to solve the COPY FREEZE issue
specifically for pgbench, I wonder if we should try to do better
and see if the limitation on a parent partition can be removed.
I can provide a patch and some benchmark numbers unless
there is something bigger I am missing about the reason this
limitation exists?
[1]: https://github.com/postgres/postgres/blob/master/src/backend/commands/copyfrom.c#L727-L735
Regards,
Sami
02.02.2025 20:45, Álvaro Herrera пишет:
On 2025-Jan-31, Melanie Plageman wrote:
Maybe instead of just not using COPY FREEZE on a table if it is
partitioned, we could add new data generation init_steps. Perhaps one
that is client-side data generation (g) but with no freezing? I'm not
really sure what the letter would be (f? making it f, g, and G?).I think it makes sense to do what you suggest, but on the other hand,
the original code that Sergey is patching looks like a hack in the sense
that it hardcodes which tables to use FREEZE with. There's no point to
doing things that way actually, so accepting Sergey's patch to replace
that with a relkind check feels sensible to me.I think the query should be
SELECT relkind FROM pg_catalog.pg_class WHERE oid='%s'::pg_catalog.regclass
if only for consistency with pgbench's other query on catalogs.Your proposal to add different init_steps might be reasonable, at least
if we allowed partitionedness of tables to vary in other ways (eg. if we
made pgbench_history partitioned), but I don't think it conflicts with
Sergey's patch in spirit.
Thanks for the note. I changed the query in the patch (v2 patch attached)
Btw, an additional benefit from the patch is that we can use foreign tables
(for example, to test postgres_fdw optimizations)
--
With best regards,
Sergey Tatarintsev,
PostgresPro
Attachments:
v1-0002-Fix-pgbench-client-side-data-generation-f.patchtext/x-patch; charset=UTF-8; name=v1-0002-Fix-pgbench-client-side-data-generation-f.patchDownload
From 9aaf2b222d6eb0c218c8a417280a8fdc0a42c296 Mon Sep 17 00:00:00 2001
From: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Date: Thu, 30 Jan 2025 14:52:29 +0700
Subject: [PATCH-v1] Fix pgbench client-side data generation for partitioned tables
Client-side data generation cannot perform COPY WITH (FREEZE = ON) on partitioned
tables except pgbench_accounts.
Patch disables FREEZE for any non-regular tables
---
src/bin/pgbench/pgbench.c | 34 +++++++++++++++++++++++++---------
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index b5cf3c6ed01..cac5312c4b1 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -847,6 +847,28 @@ static const PsqlScanCallbacks pgbench_callbacks = {
NULL, /* don't need get_variable functionality */
};
+static bool
+is_regular_table(PGconn *con, const char *table)
+{
+ PGresult *res;
+ char *sql = psprintf("SELECT relkind FROM pg_catalog.pg_class WHERE oid='%s'::pg_catalog.regclass", table);
+ bool is_regular_table = true;
+
+ res = PQexec(con, sql);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ pg_log_error("query failed: %s", PQerrorMessage(con));
+ pg_log_error_detail("Query was: %s", sql);
+ exit(1);
+ }
+ if (!PQgetisnull(res, 0, 0))
+ is_regular_table = strncmp(PQgetvalue(res, 0, 0), "r", 1) == 0;
+
+ PQclear(res);
+ pfree(sql);
+ return is_regular_table;
+}
+
static inline pg_time_usec_t
pg_time_now(void)
{
@@ -4958,16 +4980,10 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
initPQExpBuffer(&sql);
- /*
- * Use COPY with FREEZE on v14 and later for all the tables except
- * pgbench_accounts when it is partitioned.
- */
- if (PQserverVersion(con) >= 140000)
- {
- if (strcmp(table, "pgbench_accounts") != 0 ||
- partitions == 0)
+ /* Use COPY with FREEZE on v14 and later for all regular tables */
+ if ((PQserverVersion(con) >= 140000) && is_regular_table(con, table))
copy_statement_fmt = "copy %s from stdin with (freeze on)";
- }
+
n = pg_snprintf(copy_statement, sizeof(copy_statement), copy_statement_fmt, table);
if (n >= sizeof(copy_statement))
--
2.43.0
On 2025-Feb-03, Sergey Tatarintsev wrote:
Thanks for the note. I changed the query in the patch (v2 patch attached)
Btw, an additional benefit from the patch is that we can use foreign tables
(for example, to test postgres_fdw optimizations)
Good thought, and maybe it would be better if the new function was
"get_table_relkind" which just returns the char, and this check
+ /* Use COPY with FREEZE on v14 and later for all regular tables */ + if ((PQserverVersion(con) >= 140000) && is_regular_table(con, table)) copy_statement_fmt = "copy %s from stdin with (freeze on)";
can be "&& get_table_relkind(con, table) == RELKIND_RELATION"
which I think is more natural.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
03.02.2025 14:57, Álvaro Herrera пишет:
On 2025-Feb-03, Sergey Tatarintsev wrote:
Thanks for the note. I changed the query in the patch (v2 patch attached)
Btw, an additional benefit from the patch is that we can use foreign tables
(for example, to test postgres_fdw optimizations)Good thought, and maybe it would be better if the new function was
"get_table_relkind" which just returns the char, and this check+ /* Use COPY with FREEZE on v14 and later for all regular tables */ + if ((PQserverVersion(con) >= 140000) && is_regular_table(con, table)) copy_statement_fmt = "copy %s from stdin with (freeze on)";can be "&& get_table_relkind(con, table) == RELKIND_RELATION"
which I think is more natural.
sounds reasonable.
patch v3 attached
--
With best regards,
Sergey Tatarintsev,
PostgresPro
Attachments:
v3-0001-Fix-pgbench-client-side-data-generation-for.patchtext/x-patch; charset=UTF-8; name=v3-0001-Fix-pgbench-client-side-data-generation-for.patchDownload
From cdda0dcb2c0fd8d14cddd185020fd0b1ecea59be Mon Sep 17 00:00:00 2001
From: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Date: Mon, 3 Feb 2025 19:18:06 +0700
Subject: [PATCH] [PATCH-v3] Fix pgbench client-side data generation for
partitioned tables
Client-side data generation cannot perform COPY WITH (FREEZE = ON) on partitioned
tables except pgbench_accounts.
Patch disables FREEZE for any non-regular tables
---
src/bin/pgbench/pgbench.c | 36 +++++++++++++++++++++++++++---------
1 file changed, 27 insertions(+), 9 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 40592e6260..d003b407bf 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -53,6 +53,7 @@
#include <sys/select.h>
#endif
+#include "catalog/pg_class.h"
#include "common/int.h"
#include "common/logging.h"
#include "common/pg_prng.h"
@@ -848,6 +849,29 @@ static const PsqlScanCallbacks pgbench_callbacks = {
NULL, /* don't need get_variable functionality */
};
+static char
+get_table_relkind(PGconn *con, const char *table)
+{
+ PGresult *res;
+ char *sql = psprintf("SELECT relkind FROM pg_catalog.pg_class WHERE oid='%s'::pg_catalog.regclass", table);
+ char *val;
+ char relkind = RELKIND_RELATION;
+
+ res = PQexec(con, sql);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ pg_log_error("query failed: %s", PQerrorMessage(con));
+ pg_log_error_detail("Query was: %s", sql);
+ exit(1);
+ }
+ val = PQgetvalue(res, 0, 0);
+ Assert(strlen(val) == 1);
+ relkind = val[0];
+ PQclear(res);
+ pfree(sql);
+ return relkind;
+}
+
static inline pg_time_usec_t
pg_time_now(void)
{
@@ -4962,16 +4986,10 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
initPQExpBuffer(&sql);
- /*
- * Use COPY with FREEZE on v14 and later for all the tables except
- * pgbench_accounts when it is partitioned.
- */
- if (PQserverVersion(con) >= 140000)
- {
- if (strcmp(table, "pgbench_accounts") != 0 ||
- partitions == 0)
+ /* Use COPY with FREEZE on v14 and later for all regular tables */
+ if ((PQserverVersion(con) >= 140000) && get_table_relkind(con, table) == RELKIND_RELATION)
copy_statement_fmt = "copy %s from stdin with (freeze on)";
- }
+
n = pg_snprintf(copy_statement, sizeof(copy_statement), copy_statement_fmt, table);
if (n >= sizeof(copy_statement))
--
2.43.0
On Mon, Feb 3, 2025 at 7:23 AM Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
03.02.2025 14:57, Álvaro Herrera пишет:
On 2025-Feb-03, Sergey Tatarintsev wrote:
Thanks for the note. I changed the query in the patch (v2 patch attached)
Btw, an additional benefit from the patch is that we can use foreign tables
(for example, to test postgres_fdw optimizations)Good thought, and maybe it would be better if the new function was
"get_table_relkind" which just returns the char, and this check+ /* Use COPY with FREEZE on v14 and later for all regular tables */ + if ((PQserverVersion(con) >= 140000) && is_regular_table(con, table)) copy_statement_fmt = "copy %s from stdin with (freeze on)";can be "&& get_table_relkind(con, table) == RELKIND_RELATION"
which I think is more natural.
sounds reasonable.
patch v3 attached
Okay, I've stared at this a bit, and it seems basically fine the way
it is (I might add a bit more whitespace, clean up the commit message,
etc). So I'm interested in committing it. I will admit that having
never committed anything to pgbench, I'm a bit nervous. So, give me a
couple days to work up the nerve.
- Melanie
On 2025-Feb-07, Melanie Plageman wrote:
Okay, I've stared at this a bit, and it seems basically fine the way
it is (I might add a bit more whitespace, clean up the commit message,
etc). So I'm interested in committing it. I will admit that having
never committed anything to pgbench, I'm a bit nervous. So, give me a
couple days to work up the nerve.
Sounds good. I only wanted to say that I would use PQexecParams()
instead of PQexec() in the new function, avoiding the psprintf and
pfree, and also that initializing relkind to RELKIND_RELATION is strange
and probably unnecessary.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Cuando mañana llegue pelearemos segun lo que mañana exija" (Mowgli)
08.02.2025 17:32, Álvaro Herrera пишет:
On 2025-Feb-07, Melanie Plageman wrote:
Okay, I've stared at this a bit, and it seems basically fine the way
it is (I might add a bit more whitespace, clean up the commit message,
etc). So I'm interested in committing it. I will admit that having
never committed anything to pgbench, I'm a bit nervous. So, give me a
couple days to work up the nerve.Sounds good. I only wanted to say that I would use PQexecParams()
instead of PQexec() in the new function, avoiding the psprintf and
pfree, and also that initializing relkind to RELKIND_RELATION is strange
and probably unnecessary.
Let it be so. I made the suggested changes (patch v4 attached)
--
With best regards,
Sergey Tatarintsev,
PostgresPro
Attachments:
v4-0001-Fix-pgbench-client-side-data-generation-for-partitio.patchtext/x-patch; charset=UTF-8; name=v4-0001-Fix-pgbench-client-side-data-generation-for-partitio.patchDownload
From cb363a01cdff72ccde303a1a67180e96bc9e74a8 Mon Sep 17 00:00:00 2001
From: Sergey Tatarintsev <s.tatarintsev@postgrespro.ru>
Date: Mon, 3 Feb 2025 19:18:06 +0700
Subject: [PATCH] Fix pgbench client-side data generation for partitioned
tables
Client-side data generation cannot perform COPY WITH (FREEZE = ON) on partitioned
tables except pgbench_accounts.
Patch disables FREEZE for any non-regular tables
---
src/bin/pgbench/pgbench.c | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 40592e6260..8b867b45a7 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -53,6 +53,7 @@
#include <sys/select.h>
#endif
+#include "catalog/pg_class.h"
#include "common/int.h"
#include "common/logging.h"
#include "common/pg_prng.h"
@@ -848,6 +849,30 @@ static const PsqlScanCallbacks pgbench_callbacks = {
NULL, /* don't need get_variable functionality */
};
+static char
+get_table_relkind(PGconn *con, const char *table)
+{
+ PGresult *res;
+ char *val;
+ char relkind;
+ const char *sql = "SELECT relkind FROM pg_catalog.pg_class WHERE oid=$1::pg_catalog.regclass";
+ const char *params[1] = {table};
+
+ res = PQexecParams(con, sql, 1, NULL, params, NULL, NULL, 0);
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ pg_log_error("query failed: %s", PQerrorMessage(con));
+ pg_log_error_detail("Query was: %s", sql);
+ exit(1);
+ }
+ val = PQgetvalue(res, 0, 0);
+ Assert(strlen(val) == 1);
+ relkind = val[0];
+ PQclear(res);
+
+ return relkind;
+}
+
static inline pg_time_usec_t
pg_time_now(void)
{
@@ -4962,16 +4987,10 @@ initPopulateTable(PGconn *con, const char *table, int64 base,
initPQExpBuffer(&sql);
- /*
- * Use COPY with FREEZE on v14 and later for all the tables except
- * pgbench_accounts when it is partitioned.
- */
- if (PQserverVersion(con) >= 140000)
- {
- if (strcmp(table, "pgbench_accounts") != 0 ||
- partitions == 0)
+ /* Use COPY with FREEZE on v14 and later for all regular tables */
+ if ((PQserverVersion(con) >= 140000) && get_table_relkind(con, table) == RELKIND_RELATION)
copy_statement_fmt = "copy %s from stdin with (freeze on)";
- }
+
n = pg_snprintf(copy_statement, sizeof(copy_statement), copy_statement_fmt, table);
if (n >= sizeof(copy_statement))
--
2.43.0
On Tue, Feb 11, 2025 at 3:10 AM Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:
08.02.2025 17:32, Álvaro Herrera пишет:
On 2025-Feb-07, Melanie Plageman wrote:
Okay, I've stared at this a bit, and it seems basically fine the way
it is (I might add a bit more whitespace, clean up the commit message,
etc). So I'm interested in committing it. I will admit that having
never committed anything to pgbench, I'm a bit nervous. So, give me a
couple days to work up the nerve.Sounds good. I only wanted to say that I would use PQexecParams()
instead of PQexec() in the new function, avoiding the psprintf and
pfree, and also that initializing relkind to RELKIND_RELATION is strange
and probably unnecessary.Let it be so. I made the suggested changes (patch v4 attached)
I was testing with this with the intent to commit it and noticed that
it does change behavior in one way -- previously if you created an
unlogged table with the same schema as one of the pgbench tables and
then used client-side data generation, it would COPY FREEZE data into
that table. With the patch, it would no longer do that. I can live
with that, but I thought it was worth mentioning.
- Melanie
On Tue, Feb 11, 2025 at 2:29 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
On Tue, Feb 11, 2025 at 3:10 AM Sergey Tatarintsev
<s.tatarintsev@postgrespro.ru> wrote:08.02.2025 17:32, Álvaro Herrera пишет:
On 2025-Feb-07, Melanie Plageman wrote:
Okay, I've stared at this a bit, and it seems basically fine the way
it is (I might add a bit more whitespace, clean up the commit message,
etc). So I'm interested in committing it. I will admit that having
never committed anything to pgbench, I'm a bit nervous. So, give me a
couple days to work up the nerve.Sounds good. I only wanted to say that I would use PQexecParams()
instead of PQexec() in the new function, avoiding the psprintf and
pfree, and also that initializing relkind to RELKIND_RELATION is strange
and probably unnecessary.Let it be so. I made the suggested changes (patch v4 attached)
I was testing with this with the intent to commit it and noticed that
it does change behavior in one way -- previously if you created an
unlogged table with the same schema as one of the pgbench tables and
then used client-side data generation, it would COPY FREEZE data into
that table. With the patch, it would no longer do that. I can live
with that, but I thought it was worth mentioning.
I am also wondering whether or not adding a pgbench test is
appropriate. I don't see any other pgbench tests which test data
generation when DDL was done by the user. I'm thinking maybe this is
kind of a supported but undocumented use case. In which case maybe we
don't want a test for it?
- Melanie
On Tue, Feb 11, 2025 at 2:29 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
I was testing with this with the intent to commit it and noticed that
it does change behavior in one way -- previously if you created an
unlogged table with the same schema as one of the pgbench tables and
then used client-side data generation, it would COPY FREEZE data into
that table. With the patch, it would no longer do that. I can live
with that, but I thought it was worth mentioning.
whoops, ignore this! I got turned around in the
RELKIND_/RELPERSISTANCE_ macro section of pg_class.h.
Anyway, I've committed this patch. I updated the docs but didn't add a
test this time. I'm still not sure how much we want to codify our
support of pgbench loading data into different kinds of user-defined
tables.
- Melanie