Thread: Segfault when restoring -Fd dump on current HEAD
Hi, I did upgrade of my test pg. Part of this is pg_dump -Fd of each database, then upgrade binaries, then initdb, and pg_restore. But - I can't restore any database that has any data - I get segfaults. For example, with gdb: =$ gdb --args pg_restore -vvvvv -C -Fd backup-20190225074600.10361-db-depesz.dump GNU gdb (Debian 7.12-6) 7.12.0.20161007-git Copyright (C) 2016 Free Software Foundation, Inc. License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html> This is free software: you are free to change and redistribute it. There is NO WARRANTY, to the extent permitted by law. Type "show copying" and "show warranty" for details. This GDB was configured as "x86_64-linux-gnu". Type "show configuration" for configuration details. For bug reporting instructions, please see: <http://www.gnu.org/software/gdb/bugs/>. Find the GDB manual and other documentation resources online at: <http://www.gnu.org/software/gdb/documentation/>. For help, type "help". Type "apropos word" to search for commands related to "word"... Reading symbols from pg_restore...done. (gdb) run Starting program: /home/pgdba/work/bin/pg_restore -vvvvv -C -Fd backup-20190225074600.10361-db-depesz.dump [Thread debugging using libthread_db enabled] Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1". -- -- PostgreSQL database dump -- -- Dumped from database version 12devel -- Dumped by pg_dump version 12devel -- Started on 2019-02-25 07:46:01 CET SET statement_timeout = 0; SET lock_timeout = 0; SET idle_in_transaction_session_timeout = 0; SET client_encoding = 'UTF8'; SET standard_conforming_strings = on; SELECT pg_catalog.set_config('search_path', '', false); SET check_function_bodies = false; SET client_min_messages = warning; SET row_security = off; pg_restore: creating DATABASE "depesz" -- -- TOC entry 2148 (class 1262 OID 16631) -- Name: depesz; Type: DATABASE; Schema: -; Owner: depesz -- CREATE DATABASE depesz WITH TEMPLATE = template0 ENCODING = 'UTF8' LC_COLLATE = 'en_US.UTF-8' LC_CTYPE = 'en_US.UTF-8'; ALTER DATABASE depesz OWNER TO depesz; pg_restore: connecting to new database "depesz" \connect depesz SET statement_timeout = 0; SET lock_timeout = 0; SET idle_in_transaction_session_timeout = 0; SET client_encoding = 'UTF8'; SET standard_conforming_strings = on; SELECT pg_catalog.set_config('search_path', '', false); SET check_function_bodies = false; SET client_min_messages = warning; SET row_security = off; pg_restore: creating TABLE "public.test" SET default_tablespace = ''; SET default_with_oids = false; -- -- TOC entry 196 (class 1259 OID 16632) -- Name: test; Type: TABLE; Schema: public; Owner: depesz -- CREATE TABLE public.test ( i integer ); ALTER TABLE public.test OWNER TO depesz; -- -- TOC entry 2142 (class 0 OID 16632) -- Dependencies: 196 -- Data for Name: test; Type: TABLE DATA; Schema: public; Owner: depesz -- File: 2142.dat -- Program received signal SIGSEGV, Segmentation fault. 0x000055555555d99c in _printTocEntry (AH=AH@entry=0x55555577e350, te=te@entry=0x5555557844a0, isData=isData@entry=true) atpg_backup_archiver.c:3636 3636 pg_backup_archiver.c: No such file or directory. (gdb) bt #0 0x000055555555d99c in _printTocEntry (AH=AH@entry=0x55555577e350, te=te@entry=0x5555557844a0, isData=isData@entry=true)at pg_backup_archiver.c:3636 #1 0x000055555555e41d in restore_toc_entry (AH=AH@entry=0x55555577e350, te=te@entry=0x5555557844a0, is_parallel=is_parallel@entry=false)at pg_backup_archiver.c:852 #2 0x000055555555eebb in RestoreArchive (AHX=0x55555577e350) at pg_backup_archiver.c:675 #3 0x000055555555aaab in main (argc=5, argv=<optimized out>) at pg_restore.c:432 (gdb) if you'd need the dump to investigate - it's available here: https://www.depesz.com/wp-content/uploads/2019/02/bad.dump.tar.gz Unfortunately I don't have previous binaries, but I refreshed previously around a week ago. Best regards, depesz
> On Mon, Feb 25, 2019 at 8:45 AM hubert depesz lubaczewski <depesz@depesz.com> wrote: > > I did upgrade of my test pg. Part of this is pg_dump -Fd of each > database, then upgrade binaries, then initdb, and pg_restore. > > But - I can't restore any database that has any data - I get segfaults. Thank for reporting. Unfortunately, I can't reproduce this issue on the master (for me it's currently bc09d5e4cc) with the dump you've provided - do I need to do something more than just pg_restore to trigger it?
On Mon, Feb 25, 2019 at 11:01:05AM +0100, Dmitry Dolgov wrote: > > On Mon, Feb 25, 2019 at 8:45 AM hubert depesz lubaczewski <depesz@depesz.com> wrote: > > > > I did upgrade of my test pg. Part of this is pg_dump -Fd of each > > database, then upgrade binaries, then initdb, and pg_restore. > > > > But - I can't restore any database that has any data - I get segfaults. > > Thank for reporting. Unfortunately, I can't reproduce this issue on the master > (for me it's currently bc09d5e4cc) with the dump you've provided - do I need to > do something more than just pg_restore to trigger it? What's crashing for me is restoring the (12dev) dumpfile using v11 psql: [pryzbyj@database tmp]$ pg_restore backup-20190225074600.10361-db-depesz.dump >/dev/null Segmentation fault (core dumped) [pryzbyj@database tmp]$ pg_restore -V pg_restore (PostgreSQL) 11.2 I would restore dump into v12dev and re-dump and compare dump output, except it seems like pg_restore -d no longer restores into a database ?? [pryzbyj@database tmp]$ PGHOST=/tmp PGPORT=5678 PATH=~/src/postgresql.bin/bin pg_restore backup-20190225074600.10361-db-depesz.dump-d postgres |wc -l 44 Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
> What's crashing for me is restoring the (12dev) dumpfile using v11 psql:
Yeah, I can reproduce that here, using either -Fc or -Fd format dumps.
The immediate problem in your example is that the "defn" field of a
TABLE DATA entry is now null where it used to be an empty string.
Poking at related examples suggests that other fields have suffered
the same fate.
It appears to me that f831d4acc required a good deal more adult
supervision than it actually got. That was alleged to be a small
notational refactoring, not a redefinition of what gets put into
dump files.
regards, tom lane
On Mon, Feb 25, 2019 at 08:45:39AM +0100, hubert depesz lubaczewski wrote: > Hi, > I did upgrade of my test pg. Part of this is pg_dump -Fd of each > database, then upgrade binaries, then initdb, and pg_restore. Sorry, please disregard this problem. Error was sitting on a chair. Best regards, depesz
On Mon, Feb 25, 2019 at 11:20:14AM -0500, Tom Lane wrote: > It appears to me that f831d4acc required a good deal more adult > supervision than it actually got. That was alleged to be a small > notational refactoring, not a redefinition of what gets put into > dump files. How much consistent do we need to be for custom dump archives regarding backward and upward-compatibility? For dumps, we give no guarantee that a dump taken with pg_dump on version N will be compatible with a backend at version (N+1), so there is a effort for backward compatibility, not really upward compatibility. It seems to me that f831d4acc should have bumped at least MAKE_ARCHIVE_VERSION as it changes the dump contents, still it seems like a lot of for some refactoring? FWIW, I have gone through the commit's thread and I actually agree that instead of a mix of empty strings and NULL, using only NULL is cleaner. -- Michael
Attachment
Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Feb 25, 2019 at 11:20:14AM -0500, Tom Lane wrote:
>> It appears to me that f831d4acc required a good deal more adult
>> supervision than it actually got. That was alleged to be a small
>> notational refactoring, not a redefinition of what gets put into
>> dump files.
> How much consistent do we need to be for custom dump archives
> regarding backward and upward-compatibility?
Well, if we didn't want to fix this, a reasonable way to go about
it would be to bump the archive version number in pg_dump output,
so that old versions would issue a useful complaint instead of crashing.
However, I repeat that this patch was sold as a notational improvement,
not something that was going to break format compatibility. I think if
anyone had mentioned the latter, there would have been push-back against
its being committed at all. I am providing such push-back right now,
because I don't think we should break file compatibility for this.
Also, I'm now realizing that 4dbe19690 was probably not fixing an
aboriginal bug, but something that this patch introduced, because
we'd likely have noticed it before if the owner field could have
been a null pointer all along. How much do you want to bet on
whether there are other such bugs now, that we haven't yet tripped
over?
I think this patch needs to be worked over so that what it writes
is exactly what was written before. If the author is unwilling
to do that PDQ, it should be reverted.
regards, tom lane
On Tue, Feb 26, 2019 at 12:16:35AM -0500, Tom Lane wrote: > Well, if we didn't want to fix this, a reasonable way to go about > it would be to bump the archive version number in pg_dump output, > so that old versions would issue a useful complaint instead of crashing. > However, I repeat that this patch was sold as a notational improvement, > not something that was going to break format compatibility. I think if > anyone had mentioned the latter, there would have been push-back against > its being committed at all. I am providing such push-back right now, > because I don't think we should break file compatibility for this. While I agree that the patch makes handling of the different fields in archive entries cleaner, I agree as well that this is not enough to justify a dump version bump. > I think this patch needs to be worked over so that what it writes > is exactly what was written before. If the author is unwilling > to do that PDQ, it should be reverted. Works for me. With a quick read of the code, it seems to me that it is possible to keep compatibility while keeping the simplifications around ArchiveEntry()'s refactoring. Alvaro? -- Michael
Attachment
> On Tue, Feb 26, 2019 at 6:38 AM Michael Paquier <michael@paquier.xyz> wrote: > > Works for me. With a quick read of the code, it seems to me that it > is possible to keep compatibility while keeping the simplifications > around ArchiveEntry()'s refactoring. Yes, it should be rather simple, we can e.g. return to the old less consistent NULL handling approach something (like in the attached patch), or replace a NULL value with an empty string in WriteToc. Give me a moment, I'll check it out. At the same time I would suggest to keep replace_line_endings -> sanitize_line, since it doesn't break compatibility.
Attachment
> On Tue, Feb 26, 2019 at 9:13 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote: > > > On Tue, Feb 26, 2019 at 6:38 AM Michael Paquier <michael@paquier.xyz> wrote: > > > > Works for me. With a quick read of the code, it seems to me that it > > is possible to keep compatibility while keeping the simplifications > > around ArchiveEntry()'s refactoring. > > Yes, it should be rather simple, we can e.g. return to the old less consistent > NULL handling approach something (like in the attached patch), or replace a NULL > value with an empty string in WriteToc. Give me a moment, I'll check it out. At > the same time I would suggest to keep replace_line_endings -> sanitize_line, > since it doesn't break compatibility. Ok, unfortunately, I don't see any other ways, so the patch from the previous email is probably the best option (we could also check NULLs in WriteToc, but it means the same kind of inconsistency, and in this case I guess it's better to keep NULL handling as it was before). But I hope it still makes sense to consider more consisten approach here, maybe with the next dump version bump, if it's going to happen in the foreseeable future.
On 2019-Feb-26, Michael Paquier wrote: > On Tue, Feb 26, 2019 at 12:16:35AM -0500, Tom Lane wrote: > > Well, if we didn't want to fix this, a reasonable way to go about > > it would be to bump the archive version number in pg_dump output, > > so that old versions would issue a useful complaint instead of crashing. > > However, I repeat that this patch was sold as a notational improvement, > > not something that was going to break format compatibility. I think if > > anyone had mentioned the latter, there would have been push-back against > > its being committed at all. I am providing such push-back right now, > > because I don't think we should break file compatibility for this. > > While I agree that the patch makes handling of the different fields in > archive entries cleaner, I agree as well that this is not enough to > justify a dump version bump. Yeah, it was a nice thing to have, but I didn't keep in mind that we ought to be able to provide such upwards compatibility. (Is this upwards or downwards or backwards or forwards compatibility, now, actually? I can't quite figure it out which direction it goes.) > > I think this patch needs to be worked over so that what it writes > > is exactly what was written before. If the author is unwilling > > to do that PDQ, it should be reverted. > > Works for me. With a quick read of the code, it seems to me that it > is possible to keep compatibility while keeping the simplifications > around ArchiveEntry()'s refactoring. Alvaro? Yeah, let me review the patch Dmitry just sent. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2019-Feb-26, Dmitry Dolgov wrote: > Yes, it should be rather simple, we can e.g. return to the old less consistent > NULL handling approach something (like in the attached patch), or replace a NULL > value with an empty string in WriteToc. Give me a moment, I'll check it out. At > the same time I would suggest to keep replace_line_endings -> sanitize_line, > since it doesn't break compatibility. Hmm, shouldn't we modify sanitize_line so that it returns strdup(hyphen) when input is empty and want_hyphen, too? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Hmm, shouldn't we modify sanitize_line so that it returns strdup(hyphen)
> when input is empty and want_hyphen, too?
If this patch is touching the behavior of functions like that, then
it's going in the wrong direction; the need for any such change suggests
strongly that you've failed to restore the old behavior as to which TOC
fields can be null or not.
There might be reason to make such cleanups/improvements separately,
but let's *not* fuzz things up by doing them in the corrective patch.
regards, tom lane
On 2019-Feb-26, Dmitry Dolgov wrote: > > On Tue, Feb 26, 2019 at 6:38 AM Michael Paquier <michael@paquier.xyz> wrote: > > > > Works for me. With a quick read of the code, it seems to me that it > > is possible to keep compatibility while keeping the simplifications > > around ArchiveEntry()'s refactoring. > > Yes, it should be rather simple, we can e.g. return to the old less consistent > NULL handling approach something (like in the attached patch), or replace a NULL > value with an empty string in WriteToc. Give me a moment, I'll check it out. At > the same time I would suggest to keep replace_line_endings -> sanitize_line, > since it doesn't break compatibility. I think it would be better to just put back the .defn = "" (etc) to the ArchiveEntry calls. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> I think it would be better to just put back the .defn = "" (etc) to the
> ArchiveEntry calls.
Yeah, that was what I was imagining --- just make the ArchiveEntry calls
act exactly like they did before in terms of what gets filled into the
TOC fields. This episode is a fine reminder of why premature optimization
is the root of all evil...
regards, tom lane
> On Tue, Feb 26, 2019 at 11:53 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > On 2019-Feb-26, Dmitry Dolgov wrote: > > > Yes, it should be rather simple, we can e.g. return to the old less consistent > > NULL handling approach something (like in the attached patch), or replace a NULL > > value with an empty string in WriteToc. Give me a moment, I'll check it out. At > > the same time I would suggest to keep replace_line_endings -> sanitize_line, > > since it doesn't break compatibility. > > Hmm, shouldn't we modify sanitize_line so that it returns strdup(hyphen) > when input is empty and want_hyphen, too? Yes, you're right. > I think it would be better to just put back the .defn = "" (etc) to the > ArchiveEntry calls. Then we should do this not only for defn, but for owner and dropStmt too. I can update the fix patch I've sent before, if it's preferrable approach in this particular situation. But I hope there are no objections if I'll then submit the original changes with more consistent null handling separately to make decision about them more consciously.
On 2019-Feb-27, Dmitry Dolgov wrote: > > On Tue, Feb 26, 2019 at 11:53 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > > I think it would be better to just put back the .defn = "" (etc) to the > > ArchiveEntry calls. > > Then we should do this not only for defn, but for owner and dropStmt too. Yeah, absolutely. > I can > update the fix patch I've sent before, if it's preferrable approach in this > particular situation. I'm not sure we need those changes, since we're forced to update all callsites anyway. > But I hope there are no objections if I'll then submit the original > changes with more consistent null handling separately to make decision > about them more consciously. I think we should save such a patch for whenever we next update the archive version number, which could take a couple of years given past history. I'm inclined to add a comment near K_VERS_SELF to remind whoever next patches it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2019-Feb-27, Dmitry Dolgov wrote:
>> But I hope there are no objections if I'll then submit the original
>> changes with more consistent null handling separately to make decision
>> about them more consciously.
> I think we should save such a patch for whenever we next update the
> archive version number, which could take a couple of years given past
> history. I'm inclined to add a comment near K_VERS_SELF to remind
> whoever next patches it.
+1. This isn't an unreasonable cleanup idea, but being only a cleanup
idea, it doesn't seem worth creating compatibility issues for. Let's
wait till there is some more-pressing reason to change the archive format,
and then fix this in the same release cycle.
I'd also note that given what we've seen so far, there are going to be
some slow-to-flush-out null pointer dereferencing bugs from this.
I'm not really eager to introduce that towards the tail end of a devel
cycle.
regards, tom lane
> On Wed, Feb 27, 2019 at 1:32 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
>
> > > I think it would be better to just put back the .defn = "" (etc) to the
> > > ArchiveEntry calls.
> >
> > Then we should do this not only for defn, but for owner and dropStmt too.
>
> Yeah, absolutely.
Done, please find the attached patch.
> > I can
> > update the fix patch I've sent before, if it's preferrable approach in this
> > particular situation.
>
> I'm not sure we need those changes, since we're forced to update all
> callsites anyway.
I guess we can keep the part about removing null checks before using strlen,
since it's going to be useless.
> On Wed, Feb 27, 2019 at 10:36 AM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> > On Tue, Feb 26, 2019 at 11:53 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> >
> > On 2019-Feb-26, Dmitry Dolgov wrote:
> >
> > > Yes, it should be rather simple, we can e.g. return to the old less consistent
> > > NULL handling approach something (like in the attached patch), or replace a NULL
> > > value with an empty string in WriteToc. Give me a moment, I'll check it out. At
> > > the same time I would suggest to keep replace_line_endings -> sanitize_line,
> > > since it doesn't break compatibility.
> >
> > Hmm, shouldn't we modify sanitize_line so that it returns strdup(hyphen)
> > when input is empty and want_hyphen, too?
>
> Yes, you're right.
I've looked closer, and looks like I was mistaken. In the only place where it
matters we anyway pass NULL after verifying noOwner:
sanitized_owner = sanitize_line(ropt->noOwner ? NULL : te->owner, true);
So I haven't change sanitize_line yet, but can update it if there is a strong
opinion about this function.
Attachment
On Wed, Feb 27, 2019 at 12:02:43PM -0500, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: >> I think we should save such a patch for whenever we next update the >> archive version number, which could take a couple of years given past >> history. I'm inclined to add a comment near K_VERS_SELF to remind >> whoever next patches it. > > +1. This isn't an unreasonable cleanup idea, but being only a cleanup > idea, it doesn't seem worth creating compatibility issues for. Let's > wait till there is some more-pressing reason to change the archive format, > and then fix this in the same release cycle. +1. Having a comment as reminder would be really nice. -- Michael
Attachment
On 2019-Feb-27, Dmitry Dolgov wrote: > > On Wed, Feb 27, 2019 at 1:32 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > > > > I think it would be better to just put back the .defn = "" (etc) to the > > > > ArchiveEntry calls. > > > > > > Then we should do this not only for defn, but for owner and dropStmt too. > > > > Yeah, absolutely. > > Done, please find the attached patch. Pushed, thanks. I added the reminder comment I mentioned. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On Thu, Feb 28, 2019 at 9:24 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > Pushed, thanks. I added the reminder comment I mentioned. Thank you, sorry for troubles.
Hi, On 2019-02-27 09:32:17 -0300, Alvaro Herrera wrote: > On 2019-Feb-27, Dmitry Dolgov wrote: > > But I hope there are no objections if I'll then submit the original > > changes with more consistent null handling separately to make decision > > about them more consciously. > > I think we should save such a patch for whenever we next update the > archive version number, which could take a couple of years given past > history. I'm inclined to add a comment near K_VERS_SELF to remind > whoever next patches it. The pluggable storage patchset contains exactly that... I've attached the precursor patch (CREATE ACCESS METHOD ... TYPE TABLE), and the patch for pg_dump support. They need a bit more cleanup, but it might be useful information for this thread. One thing I want to bring up here rather than in the pluggable storage thread is that currently the pg_dump support for access methods deals with table access methods in a manner similar to the way we deal with tablespaces. Instead of specifying the AM on every table creation, we set the default AM when needed. That makes it easier to adjust dumps. But it does basically require breaking archive compatibility. I personally am OK with that, but I thought it might be worth discussing. I guess we could try avoid the compat issue by only increasing the archive format if there actually are any non-default AMs, but to me that doesn't seem like an improvement worthy of the necessary complications. Greetings, Andres Freund
Attachment
Andres Freund <andres@anarazel.de> writes:
> One thing I want to bring up here rather than in the pluggable storage
> thread is that currently the pg_dump support for access methods deals
> with table access methods in a manner similar to the way we deal with
> tablespaces. Instead of specifying the AM on every table creation, we
> set the default AM when needed. That makes it easier to adjust dumps.
Hm. I wonder if it'd make more sense to consider that an access method is
a property of a tablespace? That is, all tables in a tablespace have the
same access method, so you don't need to label tables individually?
> But it does basically require breaking archive compatibility. I
> personally am OK with that, but I thought it might be worth discussing.
I don't recall there being huge pushback when we did that in the past,
so I'm fine with it as long as there's an identifiable feature making
it necessary.
regards, tom lane
Hi, On 2019-03-04 13:25:40 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > One thing I want to bring up here rather than in the pluggable storage > > thread is that currently the pg_dump support for access methods deals > > with table access methods in a manner similar to the way we deal with > > tablespaces. Instead of specifying the AM on every table creation, we > > set the default AM when needed. That makes it easier to adjust dumps. > > Hm. I wonder if it'd make more sense to consider that an access method is > a property of a tablespace? That is, all tables in a tablespace have the > same access method, so you don't need to label tables individually? I don't think that'd work well. That'd basically necessitate creating multiple tablespaces just to create a table with a different AM - creating tablespaces is a superuser only activity that makes backups etc more complicated. It also doesn't correspond well to pg_class.relam etc. > > But it does basically require breaking archive compatibility. I > > personally am OK with that, but I thought it might be worth discussing. > > I don't recall there being huge pushback when we did that in the past, > so I'm fine with it as long as there's an identifiable feature making > it necessary. Cool. Greetings, Andres Freund
> On Mon, Mar 4, 2019 at 7:15 PM Andres Freund <andres@anarazel.de> wrote: > > The pluggable storage patchset contains exactly that... I've attached > the precursor patch (CREATE ACCESS METHOD ... TYPE TABLE), and the patch > for pg_dump support. They need a bit more cleanup, but it might be > useful information for this thread. Didn't expect this to happen so quickly, thanks! > On 2019-03-04 13:25:40 -0500, Tom Lane wrote: > > Andres Freund <andres@anarazel.de> writes: > > > > > > But it does basically require breaking archive compatibility. I > > > personally am OK with that, but I thought it might be worth discussing. > > > > I don't recall there being huge pushback when we did that in the past, > > so I'm fine with it as long as there's an identifiable feature making > > it necessary. > > Cool. Then I guess we need to add the attached patch on top of a pg_dump support for table am. It contains changes to use NULL as a default value for owner / defn / dropStmt (exactly what we've changed back in 19455c9f56), and doesn't crash, since K_VERS is different.
Attachment
On 2019-Mar-10, Dmitry Dolgov wrote: > Then I guess we need to add the attached patch on top of a pg_dump support for > table am. It contains changes to use NULL as a default value for owner / defn / > dropStmt (exactly what we've changed back in 19455c9f56), and doesn't crash, > since K_VERS is different. I think this is an open item; we were supposed to do it right after 3b925e905de3, but failed to notice. Would anybody be too upset if I push this patch now? CC'ed RMT. Thanks -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019-04-25 15:05:47 -0400, Alvaro Herrera wrote: > On 2019-Mar-10, Dmitry Dolgov wrote: > > > Then I guess we need to add the attached patch on top of a pg_dump support for > > table am. It contains changes to use NULL as a default value for owner / defn / > > dropStmt (exactly what we've changed back in 19455c9f56), and doesn't crash, > > since K_VERS is different. > > I think this is an open item; we were supposed to do it right after > 3b925e905de3, but failed to notice. > > Would anybody be too upset if I push this patch now? CC'ed RMT. I think that'd make sense. The rest of the RMT probably isn't awake however, so I think it'd be good to give them 24h to object. Greetings, Andres Freund
On Thu, Apr 25, 2019 at 12:54:06PM -0700, Andres Freund wrote: > I think that'd make sense. The rest of the RMT probably isn't awake > however, so I think it'd be good to give them 24h to object. It would be nice to clean all that now, so +1 from me to apply the patch. -- Michael
Attachment
On Fri, Apr 26, 2019 at 11:55:17AM +0900, Michael Paquier wrote: >On Thu, Apr 25, 2019 at 12:54:06PM -0700, Andres Freund wrote: >> I think that'd make sense. The rest of the RMT probably isn't awake >> however, so I think it'd be good to give them 24h to object. > >It would be nice to clean all that now, so +1 from me to apply the >patch. +1 from me too -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Thanks! Pushed. Marking the open item as closed too. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services