public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v52 6/7] Remove the GUC stats_temp_directory
6+ messages / 4 participants
[nested] [flat]
* [PATCH v52 6/7] Remove the GUC stats_temp_directory
@ 2020-09-29 13:59 Kyotaro Horiguchi <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Kyotaro Horiguchi @ 2020-09-29 13:59 UTC (permalink / raw)
The new stats collection system doesn't need temporary directory, so
just remove it. pg_stat_statements modified to use pg_stat directory
to store its temporary files. As the result basebackup copies the
pg_stat_statments' temporary file if exists.
---
.../pg_stat_statements/pg_stat_statements.c | 13 +++---
doc/src/sgml/backup.sgml | 2 -
doc/src/sgml/config.sgml | 19 --------
doc/src/sgml/storage.sgml | 6 ---
src/backend/postmaster/pgstat.c | 10 -----
src/backend/replication/basebackup.c | 36 ----------------
src/backend/utils/misc/guc.c | 43 -------------------
src/backend/utils/misc/postgresql.conf.sample | 1 -
src/bin/initdb/initdb.c | 1 -
src/bin/pg_rewind/filemap.c | 7 ---
src/include/pgstat.h | 3 --
src/test/perl/PostgresNode.pm | 4 --
12 files changed, 6 insertions(+), 139 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 62cccbfa44..28279f97d5 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -89,14 +89,13 @@ PG_MODULE_MAGIC;
#define PGSS_DUMP_FILE PGSTAT_STAT_PERMANENT_DIRECTORY "/pg_stat_statements.stat"
/*
- * Location of external query text file. We don't keep it in the core
- * system's stats_temp_directory. The core system can safely use that GUC
- * setting, because the statistics collector temp file paths are set only once
- * as part of changing the GUC, but pg_stat_statements has no way of avoiding
- * race conditions. Besides, we only expect modest, infrequent I/O for query
- * strings, so placing the file on a faster filesystem is not compelling.
+ * Location of external query text file. We don't keep it in the core system's
+ * pg_stats. pg_stat_statements has no way of avoiding race conditions even if
+ * the directory were specified by a GUC. Besides, we only expect modest,
+ * infrequent I/O for query strings, so placing the file on a faster filesystem
+ * is not compelling.
*/
-#define PGSS_TEXT_FILE PG_STAT_TMP_DIR "/pgss_query_texts.stat"
+#define PGSS_TEXT_FILE PGSTAT_STAT_PERMANENT_DIRECTORY "/pgss_query_texts.stat"
/* Magic number identifying the stats file format */
static const uint32 PGSS_FILE_HEADER = 0x20201218;
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index c5557d5444..875769a57e 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1155,8 +1155,6 @@ SELECT pg_stop_backup();
<filename>pg_snapshots/</filename>, <filename>pg_stat_tmp/</filename>,
and <filename>pg_subtrans/</filename> (but not the directories themselves) can be
omitted from the backup as they will be initialized on postmaster startup.
- If <xref linkend="guc-stats-temp-directory"/> is set and is under the data
- directory then the contents of that directory can also be omitted.
</para>
<para>
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 99a8664026..c0b41c8457 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -7509,25 +7509,6 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
</listitem>
</varlistentry>
- <varlistentry id="guc-stats-temp-directory" xreflabel="stats_temp_directory">
- <term><varname>stats_temp_directory</varname> (<type>string</type>)
- <indexterm>
- <primary><varname>stats_temp_directory</varname> configuration parameter</primary>
- </indexterm>
- </term>
- <listitem>
- <para>
- Sets the directory to store temporary statistics data in. This can be
- a path relative to the data directory or an absolute path. The default
- is <filename>pg_stat_tmp</filename>. Pointing this at a RAM-based
- file system will decrease physical I/O requirements and can lead to
- improved performance.
- This parameter can only be set in the <filename>postgresql.conf</filename>
- file or on the server command line.
- </para>
- </listitem>
- </varlistentry>
-
</variablelist>
</sect2>
diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 3234adb639..6bac5e075e 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -120,12 +120,6 @@ Item
subsystem</entry>
</row>
-<row>
- <entry><filename>pg_stat_tmp</filename></entry>
- <entry>Subdirectory containing temporary files for the statistics
- subsystem</entry>
-</row>
-
<row>
<entry><filename>pg_subtrans</filename></entry>
<entry>Subdirectory containing subtransaction status data</entry>
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index fe726f55f0..6a0cad034e 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -99,16 +99,6 @@ bool pgstat_track_counts = false;
int pgstat_track_functions = TRACK_FUNC_OFF;
int pgstat_track_activity_query_size = 1024;
-/* ----------
- * Built from GUC parameter
- * ----------
- */
-char *pgstat_stat_directory = NULL;
-
-/* No longer used, but will be removed with GUC */
-char *pgstat_stat_filename = NULL;
-char *pgstat_stat_tmpname = NULL;
-
/*
* WAL usage counters saved from pgWALUsage at the previous call to
* pgstat_report_wal(). This is used to calculate how much WAL usage
diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c
index fa7bdbcefa..f0d75f55bd 100644
--- a/src/backend/replication/basebackup.c
+++ b/src/backend/replication/basebackup.c
@@ -87,9 +87,6 @@ static int basebackup_read_file(int fd, char *buf, size_t nbytes, off_t offset,
/* Was the backup currently in-progress initiated in recovery mode? */
static bool backup_started_in_recovery = false;
-/* Relative path of temporary statistics directory */
-static char *statrelpath = NULL;
-
/*
* Size of each block sent into the tar stream for larger files.
*/
@@ -152,13 +149,6 @@ struct exclude_list_item
*/
static const char *const excludeDirContents[] =
{
- /*
- * Skip temporary statistics files. PG_STAT_TMP_DIR must be skipped even
- * when stats_temp_directory is set because PGSS_TEXT_FILE is always
- * created there.
- */
- PG_STAT_TMP_DIR,
-
/*
* It is generally not useful to backup the contents of this directory
* even if the intention is to restore to another primary. See backup.sgml
@@ -261,7 +251,6 @@ perform_base_backup(basebackup_options *opt)
StringInfo labelfile;
StringInfo tblspc_map_file;
backup_manifest_info manifest;
- int datadirpathlen;
List *tablespaces = NIL;
backup_total = 0;
@@ -284,8 +273,6 @@ perform_base_backup(basebackup_options *opt)
Assert(CurrentResourceOwner == NULL);
CurrentResourceOwner = ResourceOwnerCreate(NULL, "base backup");
- datadirpathlen = strlen(DataDir);
-
backup_started_in_recovery = RecoveryInProgress();
labelfile = makeStringInfo();
@@ -314,18 +301,6 @@ perform_base_backup(basebackup_options *opt)
tablespaceinfo *ti;
int tblspc_streamed = 0;
- /*
- * Calculate the relative path of temporary statistics directory in
- * order to skip the files which are located in that directory later.
- */
- if (is_absolute_path(pgstat_stat_directory) &&
- strncmp(pgstat_stat_directory, DataDir, datadirpathlen) == 0)
- statrelpath = psprintf("./%s", pgstat_stat_directory + datadirpathlen + 1);
- else if (strncmp(pgstat_stat_directory, "./", 2) != 0)
- statrelpath = psprintf("./%s", pgstat_stat_directory);
- else
- statrelpath = pgstat_stat_directory;
-
/* Add a node for the base directory at the end */
ti = palloc0(sizeof(tablespaceinfo));
ti->size = -1;
@@ -1377,17 +1352,6 @@ sendDir(const char *path, int basepathlen, bool sizeonly, List *tablespaces,
if (excludeFound)
continue;
- /*
- * Exclude contents of directory specified by statrelpath if not set
- * to the default (pg_stat_tmp) which is caught in the loop above.
- */
- if (statrelpath != NULL && strcmp(pathbuf, statrelpath) == 0)
- {
- elog(DEBUG1, "contents of directory \"%s\" excluded from backup", statrelpath);
- size += _tarWriteDir(pathbuf, basepathlen, &statbuf, sizeonly);
- continue;
- }
-
/*
* We can skip pg_wal, the WAL segments need to be fetched from the
* WAL archive anyway. But include it as an empty directory anyway, so
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index d42aea7324..dd34e032ce 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -203,7 +203,6 @@ static bool check_autovacuum_work_mem(int *newval, void **extra, GucSource sourc
static bool check_effective_io_concurrency(int *newval, void **extra, GucSource source);
static bool check_maintenance_io_concurrency(int *newval, void **extra, GucSource source);
static bool check_huge_page_size(int *newval, void **extra, GucSource source);
-static void assign_pgstat_temp_directory(const char *newval, void *extra);
static bool check_application_name(char **newval, void **extra, GucSource source);
static void assign_application_name(const char *newval, void *extra);
static bool check_cluster_name(char **newval, void **extra, GucSource source);
@@ -560,8 +559,6 @@ char *HbaFileName;
char *IdentFileName;
char *external_pid_file;
-char *pgstat_temp_directory;
-
char *application_name;
int tcp_keepalives_idle;
@@ -4375,17 +4372,6 @@ static struct config_string ConfigureNamesString[] =
NULL, NULL, NULL
},
- {
- {"stats_temp_directory", PGC_SIGHUP, STATS_ACTIVITY,
- gettext_noop("Writes temporary statistics files to the specified directory."),
- NULL,
- GUC_SUPERUSER_ONLY
- },
- &pgstat_temp_directory,
- PG_STAT_TMP_DIR,
- check_canonical_path, assign_pgstat_temp_directory, NULL
- },
-
{
{"synchronous_standby_names", PGC_SIGHUP, REPLICATION_PRIMARY,
gettext_noop("Number of synchronous standbys and list of names of potential synchronous ones."),
@@ -11791,35 +11777,6 @@ check_huge_page_size(int *newval, void **extra, GucSource source)
return true;
}
-static void
-assign_pgstat_temp_directory(const char *newval, void *extra)
-{
- /* check_canonical_path already canonicalized newval for us */
- char *dname;
- char *tname;
- char *fname;
-
- /* directory */
- dname = guc_malloc(ERROR, strlen(newval) + 1); /* runtime dir */
- sprintf(dname, "%s", newval);
-
- /* global stats */
- tname = guc_malloc(ERROR, strlen(newval) + 12); /* /global.tmp */
- sprintf(tname, "%s/global.tmp", newval);
- fname = guc_malloc(ERROR, strlen(newval) + 13); /* /global.stat */
- sprintf(fname, "%s/global.stat", newval);
-
- if (pgstat_stat_directory)
- free(pgstat_stat_directory);
- pgstat_stat_directory = dname;
- if (pgstat_stat_tmpname)
- free(pgstat_stat_tmpname);
- pgstat_stat_tmpname = tname;
- if (pgstat_stat_filename)
- free(pgstat_stat_filename);
- pgstat_stat_filename = fname;
-}
-
static bool
check_application_name(char **newval, void **extra, GucSource source)
{
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index e543f5560f..e4fb1c0cc1 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -589,7 +589,6 @@
#track_wal_io_timing = off
#track_functions = none # none, pl, all
#track_activity_query_size = 1024 # (change requires restart)
-#stats_temp_directory = 'pg_stat_tmp'
# - Monitoring -
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 3c1cf78b4f..07a00b8d0d 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -219,7 +219,6 @@ static const char *const subdirs[] = {
"pg_replslot",
"pg_tblspc",
"pg_stat",
- "pg_stat_tmp",
"pg_xact",
"pg_logical",
"pg_logical/snapshots",
diff --git a/src/bin/pg_rewind/filemap.c b/src/bin/pg_rewind/filemap.c
index 2618b4c957..ab5cb51de7 100644
--- a/src/bin/pg_rewind/filemap.c
+++ b/src/bin/pg_rewind/filemap.c
@@ -87,13 +87,6 @@ struct exclude_list_item
*/
static const char *excludeDirContents[] =
{
- /*
- * Skip temporary statistics files. PG_STAT_TMP_DIR must be skipped even
- * when stats_temp_directory is set because PGSS_TEXT_FILE is always
- * created there.
- */
- "pg_stat_tmp", /* defined as PG_STAT_TMP_DIR */
-
/*
* It is generally not useful to backup the contents of this directory
* even if the intention is to restore to another primary. See backup.sgml
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 44ef3fdc35..4e464c8528 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -33,9 +33,6 @@
#define PGSTAT_STAT_PERMANENT_FILENAME "pg_stat/saved_stats"
#define PGSTAT_STAT_PERMANENT_TMPFILE "pg_stat/saved_stats.tmp"
-/* Default directory to store temporary statistics data in */
-#define PG_STAT_TMP_DIR "pg_stat_tmp"
-
/* Values for track_functions GUC variable --- order is significant! */
typedef enum TrackFunctionsLevel
{
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index 9667f7667e..dd41a43b4e 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -455,10 +455,6 @@ sub init
print $conf TestLib::slurp_file($ENV{TEMP_CONFIG})
if defined $ENV{TEMP_CONFIG};
- # XXX Neutralize any stats_temp_directory in TEMP_CONFIG. Nodes running
- # concurrently must not share a stats_temp_directory.
- print $conf "stats_temp_directory = 'pg_stat_tmp'\n";
-
if ($params{allows_streaming})
{
if ($params{allows_streaming} eq "logical")
--
2.27.0
----Next_Part(Wed_Mar_10_17_51_37_2021_192)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
filename="v52-0007-Exclude-pg_stat-directory-from-base-backup.patch"
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: table inheritance versus column compression and storage settings
@ 2024-02-16 14:58 Tom Lane <[email protected]>
2024-02-16 18:24 ` Re: table inheritance versus column compression and storage settings Tom Lane <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2024-02-16 14:58 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; pgsql-hackers
Peter Eisentraut <[email protected]> writes:
> I have committed this. It is great to get this behavior fixed and also
> to get the internals more consistent. Thanks.
I find it surprising that the committed patch does not touch
pg_dump. Is it really true that pg_dump dumps situations with
differing compression/storage settings accurately already?
(Note that it proves little that the pg_upgrade test passes,
since if pg_dump were blind to the settings applicable to a
child table, the second dump would still be blind to them.)
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: table inheritance versus column compression and storage settings
2024-02-16 14:58 Re: table inheritance versus column compression and storage settings Tom Lane <[email protected]>
@ 2024-02-16 18:24 ` Tom Lane <[email protected]>
2024-02-19 11:34 ` Re: table inheritance versus column compression and storage settings Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2024-02-16 18:24 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; pgsql-hackers
I wrote:
> I find it surprising that the committed patch does not touch
> pg_dump. Is it really true that pg_dump dumps situations with
> differing compression/storage settings accurately already?
It's worse than I thought. Run "make installcheck" with
today's HEAD, then:
$ pg_dump -Fc regression >r.dump
$ createdb r2
$ pg_restore -d r2 r.dump
pg_restore: error: could not execute query: ERROR: column "a" inherits conflicting storage methods
HINT: To resolve the conflict, specify a storage method explicitly.
Command was: CREATE TABLE public.stchild4 (
a text
)
INHERITS (public.stparent1, public.stparent2);
ALTER TABLE ONLY public.stchild4 ALTER COLUMN a SET STORAGE MAIN;
pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
Command was: ALTER TABLE public.stchild4 OWNER TO postgres;
pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
Command was: COPY public.stchild4 (a) FROM stdin;
pg_restore: warning: errors ignored on restore: 3
What I'd intended to compare was the results of the query added to the
regression tests:
regression=# SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
WHERE (attrelid::regclass::name like 'stparent%'
OR attrelid::regclass::name like 'stchild%')
and attname = 'a'
ORDER BY 1, 2;
attrelid | attname | attstorage
-----------+---------+------------
stparent1 | a | p
stparent2 | a | x
stchild1 | a | p
stchild3 | a | m
stchild4 | a | m
stchild5 | a | x
stchild6 | a | m
(7 rows)
r2=# SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
WHERE (attrelid::regclass::name like 'stparent%'
OR attrelid::regclass::name like 'stchild%')
and attname = 'a'
ORDER BY 1, 2;
attrelid | attname | attstorage
-----------+---------+------------
stparent1 | a | p
stchild1 | a | p
stchild3 | a | m
stparent2 | a | x
stchild5 | a | p
stchild6 | a | m
(6 rows)
So not only does stchild4 fail to restore altogether, but stchild5
ends with the wrong attstorage.
This patch definitely needs more work.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: table inheritance versus column compression and storage settings
2024-02-16 14:58 Re: table inheritance versus column compression and storage settings Tom Lane <[email protected]>
2024-02-16 18:24 ` Re: table inheritance versus column compression and storage settings Tom Lane <[email protected]>
@ 2024-02-19 11:34 ` Ashutosh Bapat <[email protected]>
2024-02-20 10:21 ` Re: table inheritance versus column compression and storage settings Peter Eisentraut <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Ashutosh Bapat @ 2024-02-19 11:34 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; pgsql-hackers
On Fri, Feb 16, 2024 at 11:54 PM Tom Lane <[email protected]> wrote:
>
> I wrote:
> > I find it surprising that the committed patch does not touch
> > pg_dump. Is it really true that pg_dump dumps situations with
> > differing compression/storage settings accurately already?
>
> It's worse than I thought. Run "make installcheck" with
> today's HEAD, then:
>
> $ pg_dump -Fc regression >r.dump
> $ createdb r2
> $ pg_restore -d r2 r.dump
> pg_restore: error: could not execute query: ERROR: column "a" inherits conflicting storage methods
> HINT: To resolve the conflict, specify a storage method explicitly.
> Command was: CREATE TABLE public.stchild4 (
> a text
> )
> INHERITS (public.stparent1, public.stparent2);
> ALTER TABLE ONLY public.stchild4 ALTER COLUMN a SET STORAGE MAIN;
>
>
> pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
> Command was: ALTER TABLE public.stchild4 OWNER TO postgres;
>
> pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
> Command was: COPY public.stchild4 (a) FROM stdin;
> pg_restore: warning: errors ignored on restore: 3
Thanks for the test. Let's call this Problem1. I expected
src/bin/pg_upgrade/t/002_pg_upgrade.pl to fail in this case since it
will execute similar steps as you did. And it actually does, except
that it uses binary-upgrade mode. In that mode, INHERITed tables are
dumped in a different manner
-- For binary upgrade, set up inheritance this way.
ALTER TABLE ONLY "public"."stchild4" INHERIT "public"."stparent1";
ALTER TABLE ONLY "public"."stchild4" INHERIT "public"."stparent2";
... snip ...
ALTER TABLE ONLY "public"."stchild4" ALTER COLUMN "a" SET STORAGE MAIN;
that does not lead to the conflict and pg_upgrade does not fail.
>
>
> What I'd intended to compare was the results of the query added to the
> regression tests:
>
> regression=# SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
> WHERE (attrelid::regclass::name like 'stparent%'
> OR attrelid::regclass::name like 'stchild%')
> and attname = 'a'
> ORDER BY 1, 2;
> attrelid | attname | attstorage
> -----------+---------+------------
> stparent1 | a | p
> stparent2 | a | x
> stchild1 | a | p
> stchild3 | a | m
> stchild4 | a | m
> stchild5 | a | x
> stchild6 | a | m
> (7 rows)
>
> r2=# SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
> WHERE (attrelid::regclass::name like 'stparent%'
> OR attrelid::regclass::name like 'stchild%')
> and attname = 'a'
> ORDER BY 1, 2;
> attrelid | attname | attstorage
> -----------+---------+------------
> stparent1 | a | p
> stchild1 | a | p
> stchild3 | a | m
> stparent2 | a | x
> stchild5 | a | p
> stchild6 | a | m
> (6 rows)
>
> So not only does stchild4 fail to restore altogether, but stchild5
> ends with the wrong attstorage.
With binary-upgrade dump and restore stchild5 gets the correct storage value.
Looks like we need a test which pg_dump s regression database and
restores it without going through pg_upgrade.
I think the fix is easy one. Dump the STORAGE and COMPRESSION clauses
with CREATE TABLE for local attributes. Those for inherited attributes
will be dumped separately.
But that will not fix an existing problem described below. Let's call
it Problem2. With HEAD at commit
57f59396bb51953bb7b957780c7f1b7f67602125 (almost a month back)
$ createdb regression
$ psql -d regression
#create table par1 (a text storage plain);
#create table par2 (a text storage plain);
#create table chld (a text) inherits (par1, par2);
NOTICE: merging multiple inherited definitions of column "a"
NOTICE: merging column "a" with inherited definition
-- parent storages conflict after child creation
#alter table par1 alter column a set storage extended;
#SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
WHERE (attrelid::regclass::name like 'par%'
OR attrelid::regclass::name like 'chld%')
and attname = 'a'
ORDER BY 1, 2;
attrelid | attname | attstorage
----------+---------+------------
par1 | a | x
par2 | a | p
chld | a | x
(3 rows)
$ createdb r2
$ pg_dump -Fc regression > /tmp/r.dump
$ pg_restore -d r2 /tmp/r.dump
pg_restore: error: could not execute query: ERROR: inherited column
"a" has a storage parameter conflict
DETAIL: EXTENDED versus PLAIN
Command was: CREATE TABLE public.chld (
a text
)
INHERITS (public.par1, public.par2);
pg_restore: error: could not execute query: ERROR: relation
"public.chld" does not exist
Command was: ALTER TABLE public.chld OWNER TO ashutosh;
pg_restore: error: could not execute query: ERROR: relation
"public.chld" does not exist
Command was: COPY public.chld (a) FROM stdin;
pg_restore: warning: errors ignored on restore: 3
Fixing this requires that we dump ALTER TABLE ... ALTER COLUMN SET
STORAGE and COMPRESSION commands after all the tables (at least
children) have been created. That seems to break the way we dump the
whole table together right now. OR dump inherited tables like binary
upgrade mode.
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: table inheritance versus column compression and storage settings
2024-02-16 14:58 Re: table inheritance versus column compression and storage settings Tom Lane <[email protected]>
2024-02-16 18:24 ` Re: table inheritance versus column compression and storage settings Tom Lane <[email protected]>
2024-02-19 11:34 ` Re: table inheritance versus column compression and storage settings Ashutosh Bapat <[email protected]>
@ 2024-02-20 10:21 ` Peter Eisentraut <[email protected]>
2024-02-23 12:35 ` Re: table inheritance versus column compression and storage settings Ashutosh Bapat <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Peter Eisentraut @ 2024-02-20 10:21 UTC (permalink / raw)
To: Ashutosh Bapat <[email protected]>; Tom Lane <[email protected]>; +Cc: pgsql-hackers
I have reverted the patch for now (and re-opened the commitfest entry).
We should continue to work on this and see if we can at least try to get
the pg_dump test coverage suitable.
On 19.02.24 12:34, Ashutosh Bapat wrote:
> On Fri, Feb 16, 2024 at 11:54 PM Tom Lane <[email protected]> wrote:
>>
>> I wrote:
>>> I find it surprising that the committed patch does not touch
>>> pg_dump. Is it really true that pg_dump dumps situations with
>>> differing compression/storage settings accurately already?
>>
>> It's worse than I thought. Run "make installcheck" with
>> today's HEAD, then:
>>
>> $ pg_dump -Fc regression >r.dump
>> $ createdb r2
>> $ pg_restore -d r2 r.dump
>> pg_restore: error: could not execute query: ERROR: column "a" inherits conflicting storage methods
>> HINT: To resolve the conflict, specify a storage method explicitly.
>> Command was: CREATE TABLE public.stchild4 (
>> a text
>> )
>> INHERITS (public.stparent1, public.stparent2);
>> ALTER TABLE ONLY public.stchild4 ALTER COLUMN a SET STORAGE MAIN;
>>
>>
>> pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
>> Command was: ALTER TABLE public.stchild4 OWNER TO postgres;
>>
>> pg_restore: error: could not execute query: ERROR: relation "public.stchild4" does not exist
>> Command was: COPY public.stchild4 (a) FROM stdin;
>> pg_restore: warning: errors ignored on restore: 3
>
> Thanks for the test. Let's call this Problem1. I expected
> src/bin/pg_upgrade/t/002_pg_upgrade.pl to fail in this case since it
> will execute similar steps as you did. And it actually does, except
> that it uses binary-upgrade mode. In that mode, INHERITed tables are
> dumped in a different manner
> -- For binary upgrade, set up inheritance this way.
> ALTER TABLE ONLY "public"."stchild4" INHERIT "public"."stparent1";
> ALTER TABLE ONLY "public"."stchild4" INHERIT "public"."stparent2";
> ... snip ...
> ALTER TABLE ONLY "public"."stchild4" ALTER COLUMN "a" SET STORAGE MAIN;
>
> that does not lead to the conflict and pg_upgrade does not fail.
>
>>
>>
>> What I'd intended to compare was the results of the query added to the
>> regression tests:
>>
>> regression=# SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
>> WHERE (attrelid::regclass::name like 'stparent%'
>> OR attrelid::regclass::name like 'stchild%')
>> and attname = 'a'
>> ORDER BY 1, 2;
>> attrelid | attname | attstorage
>> -----------+---------+------------
>> stparent1 | a | p
>> stparent2 | a | x
>> stchild1 | a | p
>> stchild3 | a | m
>> stchild4 | a | m
>> stchild5 | a | x
>> stchild6 | a | m
>> (7 rows)
>>
>> r2=# SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
>> WHERE (attrelid::regclass::name like 'stparent%'
>> OR attrelid::regclass::name like 'stchild%')
>> and attname = 'a'
>> ORDER BY 1, 2;
>> attrelid | attname | attstorage
>> -----------+---------+------------
>> stparent1 | a | p
>> stchild1 | a | p
>> stchild3 | a | m
>> stparent2 | a | x
>> stchild5 | a | p
>> stchild6 | a | m
>> (6 rows)
>>
>> So not only does stchild4 fail to restore altogether, but stchild5
>> ends with the wrong attstorage.
>
> With binary-upgrade dump and restore stchild5 gets the correct storage value.
>
> Looks like we need a test which pg_dump s regression database and
> restores it without going through pg_upgrade.
>
> I think the fix is easy one. Dump the STORAGE and COMPRESSION clauses
> with CREATE TABLE for local attributes. Those for inherited attributes
> will be dumped separately.
>
> But that will not fix an existing problem described below. Let's call
> it Problem2. With HEAD at commit
> 57f59396bb51953bb7b957780c7f1b7f67602125 (almost a month back)
> $ createdb regression
> $ psql -d regression
> #create table par1 (a text storage plain);
> #create table par2 (a text storage plain);
> #create table chld (a text) inherits (par1, par2);
> NOTICE: merging multiple inherited definitions of column "a"
> NOTICE: merging column "a" with inherited definition
> -- parent storages conflict after child creation
> #alter table par1 alter column a set storage extended;
> #SELECT attrelid::regclass, attname, attstorage FROM pg_attribute
> WHERE (attrelid::regclass::name like 'par%'
> OR attrelid::regclass::name like 'chld%')
> and attname = 'a'
> ORDER BY 1, 2;
> attrelid | attname | attstorage
> ----------+---------+------------
> par1 | a | x
> par2 | a | p
> chld | a | x
> (3 rows)
>
> $ createdb r2
> $ pg_dump -Fc regression > /tmp/r.dump
> $ pg_restore -d r2 /tmp/r.dump
> pg_restore: error: could not execute query: ERROR: inherited column
> "a" has a storage parameter conflict
> DETAIL: EXTENDED versus PLAIN
> Command was: CREATE TABLE public.chld (
> a text
> )
> INHERITS (public.par1, public.par2);
>
> pg_restore: error: could not execute query: ERROR: relation
> "public.chld" does not exist
> Command was: ALTER TABLE public.chld OWNER TO ashutosh;
>
> pg_restore: error: could not execute query: ERROR: relation
> "public.chld" does not exist
> Command was: COPY public.chld (a) FROM stdin;
> pg_restore: warning: errors ignored on restore: 3
>
> Fixing this requires that we dump ALTER TABLE ... ALTER COLUMN SET
> STORAGE and COMPRESSION commands after all the tables (at least
> children) have been created. That seems to break the way we dump the
> whole table together right now. OR dump inherited tables like binary
> upgrade mode.
>
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: table inheritance versus column compression and storage settings
2024-02-16 14:58 Re: table inheritance versus column compression and storage settings Tom Lane <[email protected]>
2024-02-16 18:24 ` Re: table inheritance versus column compression and storage settings Tom Lane <[email protected]>
2024-02-19 11:34 ` Re: table inheritance versus column compression and storage settings Ashutosh Bapat <[email protected]>
2024-02-20 10:21 ` Re: table inheritance versus column compression and storage settings Peter Eisentraut <[email protected]>
@ 2024-02-23 12:35 ` Ashutosh Bapat <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Ashutosh Bapat @ 2024-02-23 12:35 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Tom Lane <[email protected]>; pgsql-hackers
On Tue, Feb 20, 2024 at 3:51 PM Peter Eisentraut <[email protected]> wrote:
>
> I have reverted the patch for now (and re-opened the commitfest entry).
> We should continue to work on this and see if we can at least try to get
> the pg_dump test coverage suitable.
>
I have started a separate thread for dump/restore test. [1].
Using that test, I found an existing bug:
Consider
CREATE TABLE cminh6 (f1 TEXT);
ALTER TABLE cminh6 INHERIT cmparent1;
f1 remains without compression even after inherit per the current code.
But pg_dump dumps it out as
CREATE TABLE cminh6 (f1 TEXT) INHERIT(cmparent1)
Because of this after restoring cminh6::f1 inherits compression of
cmparent1. So before dump cminh6::f1 has no compression and after
restore it has compression.
I am not sure how to fix this. We want inheritance children to have
their on compression. So ALTER TABLE ... INHERIT ... no passing a
compression onto child is fine. CREATE TABLE .... INHERIT ... passing
compression onto the child being created also looks fine since that's
what we do with other attributes. Only solution I see is to introduce
"none" as a special compression method to indicate "no compression"
and store that instead of NULL in attcompression column. That looks
ugly.
Similar is the case with storage.
[1] https://www.postgresql.org/message-id/[email protected]...
--
Best Wishes,
Ashutosh Bapat
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-02-23 12:35 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 13:59 [PATCH v52 6/7] Remove the GUC stats_temp_directory Kyotaro Horiguchi <[email protected]>
2024-02-16 14:58 Re: table inheritance versus column compression and storage settings Tom Lane <[email protected]>
2024-02-16 18:24 ` Re: table inheritance versus column compression and storage settings Tom Lane <[email protected]>
2024-02-19 11:34 ` Re: table inheritance versus column compression and storage settings Ashutosh Bapat <[email protected]>
2024-02-20 10:21 ` Re: table inheritance versus column compression and storage settings Peter Eisentraut <[email protected]>
2024-02-23 12:35 ` Re: table inheritance versus column compression and storage settings Ashutosh Bapat <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox