Re: Bug in reindexdb's error reporting

[Julien Rouhaud]

On Sat, May 11, 2019 at 6:04 AM Michael Paquier  wrote:
>
> On Fri, May 10, 2019 at 09:25:58PM -0400, Tom Lane wrote:
> > Michael Paquier  writes:
> > > The refactoring bits are fine for HEAD.  For back-branches I would
> > > suggest using the simplest patch of upthread.
> >
> > Makes sense to me too.  The refactoring is mostly to make future
> > additions easier, so there's not much point for back branches.
>
> For now, I have committed and back-patched all the way down the bug
> fix.

Thanks!

>  The refactoring is also kind of nice so I'll be happy to look at
> an updated patch.  At the same time, let's get rid of
> reindex_system_catalogs() and integrate it with reindex_one_database()
> with a REINDEX_SYSTEM option in the enum.  Julien, could you send a
> new version?

Yes, I had further refactoring in mind including this one (there are
also quite some parameters passed to the functions, passing a struct
instead could be worthwhile), but I thought this should be better done
after branching.

> > Right.  Also, I was imagining folding the steps together while
> > building the commands so that there's just one switch() for that,
> > along the lines of
>
> Yes, that makes sense.

Indeed.

I attach the switch refactoring that applies on top of current HEAD,
and the reindex_system_catalogs() removal in a different patch in case
that's too much during feature freeze.
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index 7d02a7f54f..64085e79e9 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -21,7 +21,8 @@ typedef enum ReindexType
 	REINDEX_DATABASE,
 	REINDEX_SCHEMA,
 	REINDEX_TABLE,
-	REINDEX_INDEX
+	REINDEX_INDEX,
+	REINDEX_SYSTEM
 }			ReindexType;
 
 static void reindex_one_database(const char *name, const char *dbname,
@@ -34,11 +35,6 @@ static void reindex_all_databases(const char *maintenance_db,
 	  const char *username, enum trivalue prompt_password,
 	  const char *progname, bool echo,
 	  bool quiet, bool verbose, bool concurrently);
-static void reindex_system_catalogs(const char *dbname,
-		const char *host, const char *port,
-		const char *username, enum trivalue prompt_password,
-		const char *progname, bool echo, bool verbose,
-		bool concurrently);
 static void help(const char *progname);
 
 int
@@ -228,8 +224,8 @@ main(int argc, char *argv[])
 dbname = get_user_name_or_exit(progname);
 		}
 
-		reindex_system_catalogs(dbname, host, port, username, prompt_password,
-progname, echo, verbose, concurrently);
+		reindex_one_database(NULL, dbname, REINDEX_SYSTEM, host, port,
+			 username, prompt_password, progname, echo, verbose, concurrently);
 	}
 	else
 	{
@@ -334,6 +330,10 @@ reindex_one_database(const char *name, const char *dbname, ReindexType type,
 			appendQualifiedRelation(&sql, name, conn, progname, echo);
 			appendPQExpBufferStr(&sql, ";");
 			break;
+		case REINDEX_SYSTEM:
+			appendPQExpBuffer(&sql, "REINDEX%s SYSTEM%s %s;", verbose_str,
+			  concurrent_str, fmtId(PQdb(conn)));
+			break;
 	}
 
 	if (!executeMaintenanceCommand(conn, sql.data, echo))
@@ -356,6 +356,10 @@ reindex_one_database(const char *name, const char *dbname, ReindexType type,
 pg_log_error("reindexing of database \"%s\" failed: %s",
 			 PQdb(conn), PQerrorMessage(conn));
 break;
+			case REINDEX_SYSTEM:
+pg_log_error("reindexing of system catalogs on database \"%s\" failed: %s",
+			 PQdb(conn), PQerrorMessage(conn));
+break;
 		}
 		PQfinish(conn);
 		exit(1);
@@ -406,41 +410,6 @@ reindex_all_databases(const char *maintenance_db,
 	PQclear(result);
 }
 
-static void
-reindex_system_catalogs(const char *dbname, const char *host, const char *port,
-		const char *username, enum trivalue prompt_password,
-		const char *progname, bool echo, bool verbose, bool concurrently)
-{
-	PGconn	   *conn;
-	PQExpBufferData sql;
-
-	conn = connectDatabase(dbname, host, port, username, prompt_password,
-		   progname, echo, false, false);
-
-	initPQExpBuffer(&sql);
-
-	appendPQExpBuffer(&sql, "REINDEX");
-
-	if (verbose)
-		appendPQExpBuffer(&sql, " (VERBOSE)");
-
-	appendPQExpBufferStr(&sql, " SYSTEM ");
-	if (concurrently)
-		appendPQExpBuffer(&sql, "CONCURRENTLY ");
-	appendPQExpBufferStr(&sql, fmtId(PQdb(conn)));
-	appendPQExpBufferChar(&sql, ';');
-
-	if (!executeMaintenanceCommand(conn, sql.data, echo))
-	{
-		pg_log_error("reindexing of system catalogs failed: %s",
-	 PQerrorMessage(conn));
-		PQfinish(conn);
-		exit(1);
-	}
-	PQfinish(conn);
-	termPQExpBuffer(&sql);
-}
-
 static void
 help(const char *progname)
 {
diff --git a/src/bin/scripts/reindexdb.c b/src/bin/scripts/reindexdb.c
index 897ad9a71a..7d02a7f54f 100644
--- a/src/bin/scripts/reindexdb.c
+++ b/src/bin/scripts/reindexdb.c
@@ -16,8 +16,16 @@
 #include "fe_utils/string_utils.h"
 
 
+typedef enum ReindexType
+{
+	REINDEX_DATABASE,
+	REINDEX_SCHEMA,
+	REINDEX_TABLE,
+	REINDEX_INDEX
+}			ReindexType;
+
 static void reindex

Re: Bug in reindexdb's error reporting

[Michael Paquier]

On Sat, May 11, 2019 at 10:28:43AM +0200, Julien Rouhaud wrote:
> I attach the switch refactoring that applies on top of current HEAD,
> and the reindex_system_catalogs() removal in a different patch in case
> that's too much during feature freeze.

Both Look fine to me at quick glance, but I have not tested them.  I
am not sure about refactoring all the options into a structure,
perhaps it depends on what kind of patch it gives.  Regarding a merge
into the tree, I think that this refactoring should wait until
REL_12_STABLE has been created.  It is no time to take risks in
destabilizing the code.

Also, as this thread's problem has been solved, perhaps it would be
better to spawn a new thread, and to add a new entry in the CF app for
the refactoring set so as it attracts the correct audience?  The
current thread topic is unfortunately misleading based on the latest
messages exchanged.
--
Michael


signature.asc
Description: PGP signature

Re: pg12 release notes

[Bruce Momjian]

On Sat, May 11, 2019 at 03:06:40AM +0100, Andrew Gierth wrote:
> > "Bruce" == Bruce Momjian  writes:
>  Bruce> Do I need more?
> 
> That isn't quite how I'd have worded it, but I'm not sure what the best
> wording is. Something like:
> 
>  * Output REAL and DOUBLE PRECISION values in shortest-exact format by
>default, and change the behavior of extra_float_digits
> 
>Previously, float values were output rounded to 6 or 15 decimals by
>default, with the number of decimals adjusted by extra_float_digits.
>The previous rounding behavior is no longer the default, and is now
>done only if extra_float_digits is set to zero or less; if the value
>is greater than zero (which it is by default), a shortest-precise
>representation is output (for a substantial performance improvement).
>This representation preserves the exact binary value when correctly
>read back in, even though the trailing digits will usually differ
>from the output generated by previous versions when
>extra_float_digits=3.
> 
> But I'm not 100% happy with this wording and am entirely open to
> suggestions for improvement.

I went with this paragraph:

This dramatically speeds up processing of floating-point values but
causes additional trailing digits to potentially be displayed.  Users
wishing to have output that is rounded to match the previous behavior
can set extra_float_digits=0, which is no longer the
default.

Improvements?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: pg12 release notes

[Bruce Momjian]

On Fri, May 10, 2019 at 07:31:21PM -0700, Peter Geoghegan wrote:
> On Fri, May 10, 2019 at 7:11 PM Bruce Momjian  wrote:
> > >  Whether or not you include more details is not what I care about here
> > > -- I *agree* that this is insignificant.
> 
> > Well, we can move the entire item up to the incompatibility section, but
> > that seems unbalanced since the incompatibility is so small relative to
> > the entire item, and it is rare, as you mentioned.  It also seems odd to
> > create a stand-alone incompatibility item that really is part of a later
> > item already in the release notes.
> 
> That is what we've always done. The list has always been very long,
> with individual items that are on average totally insignificant.
> Breaking with that pattern in this instance will be confusing to
> users.

I have no idea what you are suggesting above.

> > I think I have understood the nuances, as listed above --- I just don't
> > agree with the solution.
> 
> To be clear, I don't expect you to agree with the solution.
> 
> Another thing that you missed from my patch is that bugfix commit
> 9b10926263d831fac5758f1493c929a49b55669b shouldn't be listed.

Why should it not be listed?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: pg12 release notes

[Bruce Momjian]

On Sat, May 11, 2019 at 10:36:13AM -0400, Bruce Momjian wrote:
> On Fri, May 10, 2019 at 07:31:21PM -0700, Peter Geoghegan wrote:
> > > I think I have understood the nuances, as listed above --- I just don't
> > > agree with the solution.
> > 
> > To be clear, I don't expect you to agree with the solution.
> > 
> > Another thing that you missed from my patch is that bugfix commit
> > 9b10926263d831fac5758f1493c929a49b55669b shouldn't be listed.
> 
> Why should it not be listed?

Thinking some more, I try to aggregate all the feature addition commits
together, but often skip "cleanups" of previous feature additions.  Are
you saying that 9b10926263d831fac5758f1493c929a49b55669b is a cleanup,
and not part of the feature addition?  It was not clear to me of
9b10926263d831fac5758f1493c929a49b55669b was a further enhancement
made possible by previous commits, or a "fix" for a previous commit.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Index Skip Scan

[Dmitry Dolgov]

> On Thu, Mar 28, 2019 at 11:01 AM Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> Rebase after refactoring of nbtree insertion scankeys. But so far it's purely
> mechanical, just to make it work - I guess I'll need to try to rewrite some
> parts of the patch, that don't look natural now, accordingly.

Here is the updated version with the changes I was talking about (mostly about
readability and code cleanup). I've also added few tests for a cursor behaviour.


v15-0001-Index-skip-scan.patch
Description: Binary data

Hello

[Bhuvan Singla]

Hello!

Greetings for the day!

I am an undergraduate student at the Indian Institute of Technology,
Kanpur. I am very much interested in contributing to your organization in
the Season of Docs.

Please let me know how we can begin discussing the project.

I will be delighted to be a part of your team.

Regards,
Bhuvan Singla
Contact: Mail , LinkedIn

Re: pg12 release notes

[Peter Geoghegan]

On Sat, May 11, 2019 at 7:40 AM Bruce Momjian  wrote:
> > > > I think I have understood the nuances, as listed above --- I just don't
> > > > agree with the solution.
> > >
> > > To be clear, I don't expect you to agree with the solution.
> > >
> > > Another thing that you missed from my patch is that bugfix commit
> > > 9b10926263d831fac5758f1493c929a49b55669b shouldn't be listed.
> >
> > Why should it not be listed?
>
> Thinking some more, I try to aggregate all the feature addition commits
> together, but often skip "cleanups" of previous feature additions.  Are
> you saying that 9b10926263d831fac5758f1493c929a49b55669b is a cleanup,
> and not part of the feature addition?  It was not clear to me of
> 9b10926263d831fac5758f1493c929a49b55669b was a further enhancement
> made possible by previous commits, or a "fix" for a previous commit.

Yes. It's a bug fix that went in after feature freeze.

-- 
Peter Geoghegan

Re: pg12 release notes

[Bruce Momjian]

On Sat, May 11, 2019 at 10:28:08AM -0700, Peter Geoghegan wrote:
> On Sat, May 11, 2019 at 7:40 AM Bruce Momjian  wrote:
> > > > > I think I have understood the nuances, as listed above --- I just 
> > > > > don't
> > > > > agree with the solution.
> > > >
> > > > To be clear, I don't expect you to agree with the solution.
> > > >
> > > > Another thing that you missed from my patch is that bugfix commit
> > > > 9b10926263d831fac5758f1493c929a49b55669b shouldn't be listed.
> > >
> > > Why should it not be listed?
> >
> > Thinking some more, I try to aggregate all the feature addition commits
> > together, but often skip "cleanups" of previous feature additions.  Are
> > you saying that 9b10926263d831fac5758f1493c929a49b55669b is a cleanup,
> > and not part of the feature addition?  It was not clear to me of
> > 9b10926263d831fac5758f1493c929a49b55669b was a further enhancement
> > made possible by previous commits, or a "fix" for a previous commit.
> 
> Yes. It's a bug fix that went in after feature freeze.

OK, commit removed.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Bug in reindexdb's error reporting

[Julien Rouhaud]

On Sat, May 11, 2019 at 2:09 PM Michael Paquier  wrote:
>
> On Sat, May 11, 2019 at 10:28:43AM +0200, Julien Rouhaud wrote:
> > I attach the switch refactoring that applies on top of current HEAD,
> > and the reindex_system_catalogs() removal in a different patch in case
> > that's too much during feature freeze.
>
> Both Look fine to me at quick glance, but I have not tested them.  I
> am not sure about refactoring all the options into a structure,
> perhaps it depends on what kind of patch it gives.  Regarding a merge
> into the tree, I think that this refactoring should wait until
> REL_12_STABLE has been created.  It is no time to take risks in
> destabilizing the code.

I've run the TAP tests and it's running fine, but this should
definitely wait for branching.

> Also, as this thread's problem has been solved, perhaps it would be
> better to spawn a new thread, and to add a new entry in the CF app for
> the refactoring set so as it attracts the correct audience?  The
> current thread topic is unfortunately misleading based on the latest
> messages exchanged.

Unless someone argue it should be applied in v12, I'll do that soon.

Re: pg12 release notes

[Peter Geoghegan]

On Sat, May 11, 2019 at 11:02 AM Bruce Momjian  wrote:
> OK, commit removed.

You're mistaken -- nothing has been pushed to master in the last 3 hours.

-- 
Peter Geoghegan

Re: Unexpected "shared memory block is still in use"

[Noah Misch]

On Fri, May 10, 2019 at 04:46:40PM -0400, Tom Lane wrote:
> I wrote:
> > Will go fix/backpatch in a minute.
> 
> Done now, but while thinking more about the issue, I had an idea: why is
> it that we base the shmem key on the postmaster's port number, and not
> on the data directory's inode number?  Using the port number not only
> increases the risk of collisions (though admittedly only in testing
> situations), but it *decreases* our ability to detect real conflicts.
> Consider case where DBA wants to change the installation's port number,
> and he edits postgresql.conf, but then uses "kill -9 && rm postmaster.pid"
> rather than some saner way of stopping the old postmaster.  When he
> starts the new one, it won't detect any remaining children of the old
> postmaster because it'll be looking in the wrong range of shmem keys.
> It seems like something tied to the data directory's identity would
> be much more trustworthy.

Good point.  Since we now ignore (SHMSTATE_FOREIGN) any segment that bears
(st_dev,st_ino) not matching $PGDATA, the change you describe couldn't make us
fail to detect a real conflict or miss a cleanup opportunity.  It would reduce
the ability to test sysv_shmem.c; I suppose one could add a debug GUC to
override the start of the key space.

> I think the reason for doing it this way originally was to allow
> one to identify which shmem segment is which in "ipcs -m" output.
> But that was back when having to clean up shmem segments manually
> was still a common task.  It's been a long time since I can remember
> needing to figure out which was which.

I don't see that presenting a problem these days, agreed.

Re: pg12 release notes

[Bruce Momjian]

On Sat, May 11, 2019 at 11:47:42AM -0700, Peter Geoghegan wrote:
> On Sat, May 11, 2019 at 11:02 AM Bruce Momjian  wrote:
> > OK, commit removed.
> 
> You're mistaken -- nothing has been pushed to master in the last 3 hours.

I am not mistaken.  I have removed it from my local copy, but have not
pushed it yet since I am adding links to the docs.  It will be done
today.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

PG 12 draft release notes

[Bruce Momjian]

I have posted a draft copy of the PG 12 release notes here:

http://momjian.us/pgsql_docs/release-12.html

They are committed to git.  It includes links to the main docs, where
appropriate.  Our official developer docs will rebuild in a few hours.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +

Re: Bug in reindexdb's error reporting

[Alvaro Herrera]

On 2019-May-11, Julien Rouhaud wrote:

> On Sat, May 11, 2019 at 2:09 PM Michael Paquier  wrote:

> > Also, as this thread's problem has been solved, perhaps it would be
> > better to spawn a new thread, and to add a new entry in the CF app for
> > the refactoring set so as it attracts the correct audience?  The
> > current thread topic is unfortunately misleading based on the latest
> > messages exchanged.
> 
> Unless someone argue it should be applied in v12, I'll do that soon.

Certainly not.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Wrong dead return value in jsonb_utils.c

[Rikard Falkeborn]

Returning -1 from a function with bool as return value is the same as
returning true. Now, the code is dead (since elog(ERROR, ...) does not
return) so it doesn't matter to the compiler, but changing to false is less
confusing for the programmer. Appologies if this is seen as unnecessary
churn.

The same code is present since 9.4, but perhaps it's not really worth
backporting since it is more of an aesthetic change?
From 11611a4c0268379b45e11faf50c927c5997d70ee Mon Sep 17 00:00:00 2001
From: Rikard Falkeborn 
Date: Sun, 12 May 2019 00:08:30 +0200
Subject: [PATCH] Fix dead code return value in jsonb_util

Returning -1 in a function with bool as return value is the same as
returning true. The change is in dead code since elog(ERROR, ...) never
returns, but changing to false remove some potential confusion.
---
 src/backend/utils/adt/jsonb_util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index 3b249fe8cb..1a28d75c59 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -1318,7 +1318,7 @@ equalsJsonbScalarValue(JsonbValue *aScalar, JsonbValue *bScalar)
 		}
 	}
 	elog(ERROR, "jsonb scalar type mismatch");
-	return -1;
+	return false;
 }
 
 /*
-- 
2.21.0

Re: Augment every test postgresql.conf

[Noah Misch]

On Sun, Apr 07, 2019 at 07:56:02AM -0400, Andrew Dunstan wrote:
> On Sun, Apr 7, 2019 at 2:41 AM Noah Misch  wrote:
> >
> > On Sun, Dec 30, 2018 at 10:32:31AM -0500, Andrew Dunstan wrote:
> > > On 12/30/18 12:53 AM, Noah Misch wrote:
> > > > 2. stats_temp_directory is incompatible with TAP suites that start more 
> > > > than
> > > >one node simultaneously.
> >
> > > The obvious quick fix would be to have PostgresNode.pm set this to the
> > > default after inserting the TEMP_CONFIG file.
> >
> > I'd like to get $SUBJECT in place for variables other than
> > stats_temp_directory, using your quick fix idea.  Attached.  When its time
> > comes, your stats_temp_directory work can delete that section.
> 
> Looks good.

Pushed.  This broke 010_dump_connstr.pl on bowerbird, introducing 'invalid
byte sequence for encoding "UTF8"' errors.  That's because log_connections
renders this 010_dump_connstr.pl solution insufficient:

  # In a SQL_ASCII database, pgwin32_message_to_UTF16() needs to
  # interpret everything as UTF8.  We're going to use byte sequences
  # that aren't valid UTF-8 strings, so that would fail.  Use LATIN1,
  # which accepts any byte and has a conversion from each byte to UTF-8.
  $ENV{LC_ALL}   = 'C';
  $ENV{PGCLIENTENCODING} = 'LATIN1';

The log_connections message prints before CheckMyDatabase() calls
pg_perm_setlocale() to activate that LATIN1 database encoding.  Since
bowerbird does a non-NLS build, GetMessageEncoding()==PG_SQL_ASCII at that
time.  Some options:

1. Make this one test explicitly set log_connections = off.  This workaround
   restores what we had a day ago.

2. Move the log_connections message after CheckMyDatabase() calls
   pg_perm_setlocale(), so it gets regular post-startup encoding treatment.
   That fixes this particular test.  It's still wrong when a database's name
   is not valid in that database's encoding.

3. If GetMessageEncoding()==PG_SQL_ASCII, make pgwin32_message_to_UTF16()
   assume the text is already UTF8, like it does when not in a transaction.
   If UTF8->UTF16 conversion fails, the caller will send untranslated bytes to
   write() or ReportEventA().

4. If GetMessageEncoding()==PG_SQL_ASCII, make pgwin32_message_to_UTF16()
   return NULL.  The caller will always send untranslated bytes to write() or
   ReportEventA().  This seems consistent with the SQL_ASCII concept and with
   pg_do_encoding_conversion()'s interpretation of SQL_ASCII.

5. When including a datname or rolname value in a message, hex-escape
   non-ASCII bytes.  They are byte sequences, not text of known encoding.
   This preserves the most information, but it's overkill and ugly in the
   probably-common case of one encoding across all databases of a cluster.

I'm inclined to do (1) in back branches and (4) in HEAD only.  (If starting
fresh today, I would store the encoding of each rolname and dbname or just use
UTF8 for those particular fields.)  Other preferences?

Thanks,
nm

Re: Augment every test postgresql.conf

[Tom Lane]

Noah Misch  writes:
> Pushed.  This broke 010_dump_connstr.pl on bowerbird, introducing 'invalid
> byte sequence for encoding "UTF8"' errors.  That's because log_connections
> renders this 010_dump_connstr.pl solution insufficient:

Ugh.

> 4. If GetMessageEncoding()==PG_SQL_ASCII, make pgwin32_message_to_UTF16()
>return NULL.  The caller will always send untranslated bytes to write() or
>ReportEventA().  This seems consistent with the SQL_ASCII concept and with
>pg_do_encoding_conversion()'s interpretation of SQL_ASCII.

> 5. When including a datname or rolname value in a message, hex-escape
>non-ASCII bytes.  They are byte sequences, not text of known encoding.
>This preserves the most information, but it's overkill and ugly in the
>probably-common case of one encoding across all databases of a cluster.

> I'm inclined to do (1) in back branches and (4) in HEAD only.  (If starting
> fresh today, I would store the encoding of each rolname and dbname or just use
> UTF8 for those particular fields.)  Other preferences?

I agree that (4) is a fairly reasonable thing to do, and wouldn't mind
back-patching that.  Taking a wider view, this seems closely related
to something I've been thinking about in connection with the recent
pg_stat_activity contretemps: that mechanism is also shoving strings
across database boundaries without a lot of worry about encodings.
Maybe we should try to develop a common solution.

One difference from the datname/rolname situation is that for
pg_stat_activity we can know the source encoding --- we aren't storing
it now, but we easily could.  If we're thinking of a future solution
only, adding a "name encoding" field to relevant shared catalogs makes
sense perhaps.  Alternatively, requiring names in shared catalogs to be
UTF8 might be a reasonable answer too.

In all these cases, throwing an error when we can't translate a character
into the destination encoding is not very pleasant.  For pg_stat_activity,
I was imagining that translating such characters to '?' might be the best
answer.  I don't know if we can get away with that for the datname/rolname
case --- at the very least, it opens problems with apparent duplication of
names that should be unique.  I don't much like your hex-encoding answer,
though; that has its own uniqueness-violation hazards, plus it's ugly.

I don't have a strong feeling about what's best.

regards, tom lane

Re: POC: Cleaning up orphaned files using undo logs

[Dilip Kumar]

On Thu, May 9, 2019 at 12:04 PM Dilip Kumar  wrote:
>
> On Mon, May 6, 2019 at 5:43 PM Dilip Kumar  wrote:
> >
> > Just for tracking, open comments which still needs to be worked on.
> >
> > 1.  Avoid special case in UndoRecordIsValid.
> > > Can we instead eliminate the special case?  It seems like the if
> > > (log->oldest_data == InvalidUndoRecPtr) case will be taken very
> > > rarely, so if it's buggy, we might not notice.
>
> I have worked on this comments and added changes in the latest patch.
> >
> > 2. While updating the previous transaction header instead of unpacking
> > complete header and writing it back, we can just unpack main header
> > and calculate the offset of uur_next and then update it directly.
>
> For this as you suggested I am not changing, updated the comments.
> >
> > 3. unifying uur_xid and uur_xidepoch into uur_fxid.
> Still open.
>
> I have also added the README.
>
> Patches can be applied on top of undo branch [1] commit:
> (cb777466d008e656f03771cf16ec7ef9d6f2778b)
>
> [1] https://github.com/EnterpriseDB/zheap/tree/undo
>

I have removed some of the globals and also improved some comments.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


0002-Provide-interfaces-to-store-and-fetch-undo-records_v6.patch
Description: Binary data