Обсуждение: Verbose output of pg_dump not show schema name
Hi all,
There are some reason to verbose output of pg_dump don't show schema name?
A output example of using "pg_dump -Fd -j8 -v"There are some reason to verbose output of pg_dump don't show schema name?
...
pg_dump: dumping contents of table geocoordinate
pg_dump: dumping contents of table historyvalue
pg_dump: dumping contents of table historyvalue
pg_dump: dumping contents of table transactionlog
pg_dump: dumping contents of table historyvalue
pg_dump: dumping contents of table historyvalue
pg_dump: dumping contents of table historyvalue
pg_dump: dumping contents of table historyvalue
...
This database have a lot of different schemas with same structure and if I need do view the status of dump I don't know what schema the table are dump from.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
On Thu, Apr 17, 2014 at 11:41 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > Hi all, > > There are some reason to verbose output of pg_dump don't show schema name? > > A output example of using "pg_dump -Fd -j8 -v" Specifying a target directory with "-f" is better here... > This database have a lot of different schemas with same structure and if I > need do view the status of dump I don't know what schema the table are dump > from. Yes this may be helpful. The attached quick'n dirty patch implements it. -- Michael
Вложения
On Thu, Apr 17, 2014 at 2:14 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> On Thu, Apr 17, 2014 at 11:41 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > Hi all,
> >
> > There are some reason to verbose output of pg_dump don't show schema name?
> >
> > A output example of using "pg_dump -Fd -j8 -v"
> Specifying a target directory with "-f" is better here...
>
> > This database have a lot of different schemas with same structure and if I
> > need do view the status of dump I don't know what schema the table are dump
> > from.
> Yes this may be helpful. The attached quick'n dirty patch implements it.
>
Very nice... thanks!!!
I add schema name do the following messages too:
pg_restore: processing data for table "public"."bar"
pg_restore: processing data for table "public"."foo"
pg_restore: processing data for table "s1"."bar"
pg_restore: processing data for table "s1"."foo"
pg_restore: processing data for table "s2"."bar"
pg_restore: processing data for table "s2"."foo"
pg_restore: processing data for table "s3"."bar"
pg_restore: processing data for table "s3"."foo"
I add schema name do the following messages too:
pg_restore: processing data for table "public"."bar"
pg_restore: processing data for table "public"."foo"
pg_restore: processing data for table "s1"."bar"
pg_restore: processing data for table "s1"."foo"
pg_restore: processing data for table "s2"."bar"
pg_restore: processing data for table "s2"."foo"
pg_restore: processing data for table "s3"."bar"
pg_restore: processing data for table "s3"."foo"
And:
pg_dump: finding the columns and types of table "s1"."foo"
pg_dump: finding the columns and types of table "s1"."bar"
pg_dump: finding the columns and types of table "s2"."foo"
pg_dump: finding the columns and types of table "s2"."bar"
pg_dump: finding the columns and types of table "s3"."foo"
pg_dump: finding the columns and types of table "s3"."bar"
pg_dump: finding the columns and types of table "public"."foo"
pg_dump: finding the columns and types of table "public"."bar"
pg_dump: finding the columns and types of table "s1"."foo"
pg_dump: finding the columns and types of table "s1"."bar"
pg_dump: finding the columns and types of table "s2"."foo"
pg_dump: finding the columns and types of table "s2"."bar"
pg_dump: finding the columns and types of table "s3"."foo"
pg_dump: finding the columns and types of table "s3"."bar"
pg_dump: finding the columns and types of table "public"."foo"
pg_dump: finding the columns and types of table "public"."bar"
And:
pg_dump: processing data for table "public"."bar"
pg_dump: dumping contents of table public.bar
pg_dump: processing data for table "public"."foo"
pg_dump: dumping contents of table public.foo
pg_dump: processing data for table "s1"."bar"
pg_dump: dumping contents of table s1.bar
pg_dump: processing data for table "s1"."foo"
pg_dump: dumping contents of table s1.foo
pg_dump: processing data for table "s2"."bar"
pg_dump: dumping contents of table s2.bar
pg_dump: processing data for table "s2"."foo"
pg_dump: dumping contents of table s2.foo
pg_dump: processing data for table "s3"."bar"
pg_dump: dumping contents of table s3.bar
pg_dump: processing data for table "s3"."foo"
pg_dump: dumping contents of table s3.foo
pg_dump: processing data for table "public"."bar"
pg_dump: dumping contents of table public.bar
pg_dump: processing data for table "public"."foo"
pg_dump: dumping contents of table public.foo
pg_dump: processing data for table "s1"."bar"
pg_dump: dumping contents of table s1.bar
pg_dump: processing data for table "s1"."foo"
pg_dump: dumping contents of table s1.foo
pg_dump: processing data for table "s2"."bar"
pg_dump: dumping contents of table s2.bar
pg_dump: processing data for table "s2"."foo"
pg_dump: dumping contents of table s2.foo
pg_dump: processing data for table "s3"."bar"
pg_dump: dumping contents of table s3.bar
pg_dump: processing data for table "s3"."foo"
pg_dump: dumping contents of table s3.foo
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Вложения
On Thu, Apr 17, 2014 at 11:29:03AM -0300, Fabrízio de Royes Mello wrote: > > > This database have a lot of different schemas with same structure and if I > > > need do view the status of dump I don't know what schema the table are dump > > > from. > > Yes this may be helpful. The attached quick'n dirty patch implements it. > > > > Very nice... thanks!!! > > I add schema name do the following messages too: > > pg_restore: processing data for table "public"."bar" > pg_restore: processing data for table "public"."foo" > pg_restore: processing data for table "s1"."bar" > pg_restore: processing data for table "s1"."foo" > pg_restore: processing data for table "s2"."bar" > pg_restore: processing data for table "s2"."foo" > pg_restore: processing data for table "s3"."bar" > pg_restore: processing data for table "s3"."foo" Can you get that to _conditionally_ double-quote the strings? In fact, maybe we don't even need the double-quotes. How do we double-quote other places? -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Thu, Apr 17, 2014 at 11:36 AM, Bruce Momjian <bruce@momjian.us> wrote:
>
> On Thu, Apr 17, 2014 at 11:29:03AM -0300, Fabrízio de Royes Mello wrote:
> > > > This database have a lot of different schemas with same structure and if I
> > > > need do view the status of dump I don't know what schema the table are dump
> > > > from.
> > > Yes this may be helpful. The attached quick'n dirty patch implements it.
> > >
> >
> > Very nice... thanks!!!
> >
> > I add schema name do the following messages too:
> >
> > pg_restore: processing data for table "public"."bar"
> > pg_restore: processing data for table "public"."foo"
> > pg_restore: processing data for table "s1"."bar"
> > pg_restore: processing data for table "s1"."foo"
> > pg_restore: processing data for table "s2"."bar"
> > pg_restore: processing data for table "s2"."foo"
> > pg_restore: processing data for table "s3"."bar"
> > pg_restore: processing data for table "s3"."foo"
>
> Can you get that to _conditionally_ double-quote the strings?
Sorry, I didn't understand what you means? Your idea is to check if the namespace is available and then don't show the double-quote, is that?
> In fact,
> maybe we don't even need the double-quotes. How do we double-quote
> other places?
>
$ grep 'table \\\"%s' src/bin/pg_dump/*.c
src/bin/pg_dump/common.c: write_msg(NULL, "failed sanity check, parent OID %u of table \"%s\" (OID %u) not found\n",
src/bin/pg_dump/pg_backup_archiver.c: ahlog(AH, 1, "processing data for table \"%s\".\"%s\"\n",
src/bin/pg_dump/pg_backup_archiver.c: ahlog(AH, 1, "table \"%s\" could not be created, will not restore its data\n",
src/bin/pg_dump/pg_backup_db.c: warn_or_exit_horribly(AH, modulename, "COPY failed for table \"%s\": %s",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "Dumping the contents of table \"%s\" failed: PQgetCopyData() failed.\n", classname);
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "Dumping the contents of table \"%s\" failed: PQgetResult() failed.\n", classname);
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "WARNING: owner of table \"%s\" appears to be invalid\n",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "reading indexes for table \"%s\"\n",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "reading foreign key constraints for table \"%s\"\n",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "reading triggers for table \"%s\"\n",
src/bin/pg_dump/pg_dump.c: exit_horribly(NULL, "query produced null referenced table name for foreign key trigger \"%s\" on table \"%s\" (OID of table: %u)\n",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "finding the columns and types of table \"%s\".\"%s\"\n",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "finding the columns and types of table \"%s\"\n",
src/bin/pg_dump/pg_dump.c: "invalid column numbering in table \"%s\"\n",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "finding default expressions of table \"%s\"\n",
src/bin/pg_dump/pg_dump.c: "invalid adnum value %d for table \"%s\"\n",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "finding check constraints for table \"%s\"\n",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, ngettext("expected %d check constraint on table \"%s\" but found %d\n",
src/bin/pg_dump/pg_dump.c: "expected %d check constraints on table \"%s\" but found %d\n",
src/bin/pg_dump/pg_dump.c: exit_horribly(NULL, "invalid column number %d for table \"%s\"\n",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "invalid argument string (%s) for trigger \"%s\" on table \"%s\"\n",
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "query to get rule \"%s\" for table \"%s\" failed: wrong number of rows returned\n",
Just the "dumping contents of table.." message isn't double-quoted:
$ grep 'table %s' src/bin/pg_dump/*.c
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "dumping contents of table %s\n",
$ grep 'table %s' src/bin/pg_dump/*.c
src/bin/pg_dump/pg_dump.c: write_msg(NULL, "dumping contents of table %s\n",
So maybe we must double-quote of all string, i.e. "public.foo", including the missing bellow.
Regards,
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
On Thu, Apr 17, 2014 at 12:07:39PM -0300, Fabrízio de Royes Mello wrote: > > Can you get that to _conditionally_ double-quote the strings? > > Sorry, I didn't understand what you means? Your idea is to check if the > namespace is available and then don't show the double-quote, is that? The idea is that we only need quotes when there are odd characters in the identifier. We do that right now in some places, though I can't find them in pg_dump. I know psql does that, see quote_ident(). > > In fact, > > maybe we don't even need the double-quotes. How do we double-quote > > other places? > > > > Checking that more deeply I found some other places that show the table name > and all of them are double-quoted. OK. > Just the "dumping contents of table.." message isn't double-quoted: > > $ grep 'table %s' src/bin/pg_dump/*.c > src/bin/pg_dump/pg_dump.c: write_msg(NULL, "dumping contents of > table %s\n", > > > So maybe we must double-quote of all string, i.e. "public.foo", including the > missing bellow. No, I think double-quoting each part is the correct way. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
Bruce Momjian <bruce@momjian.us> writes: > The idea is that we only need quotes when there are odd characters in > the identifier. We do that right now in some places, though I can't > find them in pg_dump. I know psql does that, see quote_ident(). I think our general style rule is that identifiers embedded in messages are always double-quoted. There's an exception for type names, but not otherwise. You're confusing the message case with printing SQL. regards, tom lane
On Thu, Apr 17, 2014 at 11:44:37AM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > The idea is that we only need quotes when there are odd characters in > > the identifier. We do that right now in some places, though I can't > > find them in pg_dump. I know psql does that, see quote_ident(). > > I think our general style rule is that identifiers embedded in messages > are always double-quoted. There's an exception for type names, but > not otherwise. You're confusing the message case with printing SQL. OK. I was unclear if a status _display_ was a message like an error message. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. +
On Thu, Apr 17, 2014 at 12:46 PM, Bruce Momjian <bruce@momjian.us> wrote:
>
> On Thu, Apr 17, 2014 at 11:44:37AM -0400, Tom Lane wrote:
> > Bruce Momjian <bruce@momjian.us> writes:
> > > The idea is that we only need quotes when there are odd characters in
> > > the identifier. We do that right now in some places, though I can't
> > > find them in pg_dump. I know psql does that, see quote_ident().
> >
> > I think our general style rule is that identifiers embedded in messages
> > are always double-quoted. There's an exception for type names, but
> > not otherwise. You're confusing the message case with printing SQL.
>
> OK. I was unclear if a status _display_ was a message like an error
> message.
>
- "reading indexes for table \"%s\".\"%s\"\n"
- "reading foreign key constraints for table \"%s\".\"%s\"\n"
- "reading triggers for table \"%s\".\"%s\"\n"
- "finding the columns and types of table \"%s\".\"%s\"\n"
- "finding default expressions of table \"%s\".\"%s\"\n"
- "finding check constraints for table \"%s\".\"%s\"\n"
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Вложения
On Fri, Apr 18, 2014 at 4:29 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > > On Thu, Apr 17, 2014 at 12:46 PM, Bruce Momjian <bruce@momjian.us> wrote: >> >> On Thu, Apr 17, 2014 at 11:44:37AM -0400, Tom Lane wrote: >> > Bruce Momjian <bruce@momjian.us> writes: >> > > The idea is that we only need quotes when there are odd characters in >> > > the identifier. We do that right now in some places, though I can't >> > > find them in pg_dump. I know psql does that, see quote_ident(). >> > >> > I think our general style rule is that identifiers embedded in messages >> > are always double-quoted. There's an exception for type names, but >> > not otherwise. You're confusing the message case with printing SQL. >> >> OK. I was unclear if a status _display_ was a message like an error >> message. >> > > The attached patch fix missing double-quoted in "dumping contents of > table.." message and add schema name to other messages: > - "reading indexes for table \"%s\".\"%s\"\n" > - "reading foreign key constraints for table \"%s\".\"%s\"\n" > - "reading triggers for table \"%s\".\"%s\"\n" > > - "finding the columns and types of table \"%s\".\"%s\"\n" > - "finding default expressions of table \"%s\".\"%s\"\n" > - "finding check constraints for table \"%s\".\"%s\"\n" Cool additions. There may be a more elegant way to check if namespace is NULL, but I couldn't come up with one myself. So patch may be fine. -- Michael
<div dir="ltr"><br />On Thu, Apr 17, 2014 at 9:15 PM, Michael Paquier <<a href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>wrote:<br />><br />> On Fri, Apr 18, 2014at 4:29 AM, Fabrízio de Royes Mello<br /> > <<a href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>>wrote:<br />> ><br />> > On Thu, Apr 17,2014 at 12:46 PM, Bruce Momjian <<a href="mailto:bruce@momjian.us">bruce@momjian.us</a>> wrote:<br /> > >><br/>> >> On Thu, Apr 17, 2014 at 11:44:37AM -0400, Tom Lane wrote:<br />> >> > Bruce Momjian<<a href="mailto:bruce@momjian.us">bruce@momjian.us</a>> writes:<br />> >> > > The idea is thatwe only need quotes when there are odd characters in<br /> > >> > > the identifier. We do that rightnow in some places, though I can't<br />> >> > > find them in pg_dump. I know psql does that, see quote_ident().<br/>> >> ><br />> >> > I think our general style rule is that identifiers embeddedin messages<br /> > >> > are always double-quoted. There's an exception for type names, but<br />>>> > not otherwise. You're confusing the message case with printing SQL.<br />> >><br />> >>OK. I was unclear if a status _display_ was a message like an error<br /> > >> message.<br />> >><br/>> ><br />> > The attached patch fix missing double-quoted in "dumping contents of<br />> >table.." message and add schema name to other messages:<br />> > - "reading indexes for table \"%s\".\"%s\"\n"<br/> > > - "reading foreign key constraints for table \"%s\".\"%s\"\n"<br />> > - "reading triggersfor table \"%s\".\"%s\"\n"<br />> ><br />> > - "finding the columns and types of table \"%s\".\"%s\"\n"<br/> > > - "finding default expressions of table \"%s\".\"%s\"\n"<br />> > - "finding checkconstraints for table \"%s\".\"%s\"\n"<br />> Cool additions. There may be a more elegant way to check if namespace<br/> > is NULL, but I couldn't come up with one myself. So patch may be fine.<br />><br /><br />Hi all,<br/><br />I think this small patch was lost. There are something wrong?<br /><br />Regards,<br /><br />--<br />Fabríziode Royes Mello<br /> Consultoria/Coaching PostgreSQL<br />>> Timbira: <a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog sobre TI: <a href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/> >> Perfil Linkedin: <a href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div>
On Wed, Jul 23, 2014 at 5:48 PM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > > On Thu, Apr 17, 2014 at 9:15 PM, Michael Paquier <michael.paquier@gmail.com> > wrote: >> >> On Fri, Apr 18, 2014 at 4:29 AM, Fabrízio de Royes Mello >> <fabriziomello@gmail.com> wrote: >> > >> > On Thu, Apr 17, 2014 at 12:46 PM, Bruce Momjian <bruce@momjian.us> >> > wrote: >> >> >> >> On Thu, Apr 17, 2014 at 11:44:37AM -0400, Tom Lane wrote: >> >> > Bruce Momjian <bruce@momjian.us> writes: >> >> > > The idea is that we only need quotes when there are odd characters >> >> > > in >> >> > > the identifier. We do that right now in some places, though I >> >> > > can't >> >> > > find them in pg_dump. I know psql does that, see quote_ident(). >> >> > >> >> > I think our general style rule is that identifiers embedded in >> >> > messages >> >> > are always double-quoted. There's an exception for type names, but >> >> > not otherwise. You're confusing the message case with printing SQL. >> >> >> >> OK. I was unclear if a status _display_ was a message like an error >> >> message. >> >> >> > >> > The attached patch fix missing double-quoted in "dumping contents of >> > table.." message and add schema name to other messages: >> > - "reading indexes for table \"%s\".\"%s\"\n" >> > - "reading foreign key constraints for table \"%s\".\"%s\"\n" >> > - "reading triggers for table \"%s\".\"%s\"\n" >> > >> > - "finding the columns and types of table \"%s\".\"%s\"\n" >> > - "finding default expressions of table \"%s\".\"%s\"\n" >> > - "finding check constraints for table \"%s\".\"%s\"\n" >> Cool additions. There may be a more elegant way to check if namespace >> is NULL, but I couldn't come up with one myself. So patch may be fine. >> > > Hi all, > > I think this small patch was lost. There are something wrong? Did it get added to a CommitFest? I don't see it there. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Thu, Jul 24, 2014 at 4:36 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Jul 23, 2014 at 5:48 PM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> >
> > On Thu, Apr 17, 2014 at 9:15 PM, Michael Paquier <michael.paquier@gmail.com>
> > wrote:
> >>
> >> On Fri, Apr 18, 2014 at 4:29 AM, Fabrízio de Royes Mello
> >> <fabriziomello@gmail.com> wrote:
> >> >
> >> > On Thu, Apr 17, 2014 at 12:46 PM, Bruce Momjian <bruce@momjian.us>
> >> > wrote:
> >> >>
> >> >> On Thu, Apr 17, 2014 at 11:44:37AM -0400, Tom Lane wrote:
> >> >> > Bruce Momjian <bruce@momjian.us> writes:
> >> >> > > The idea is that we only need quotes when there are odd characters
> >> >> > > in
> >> >> > > the identifier. We do that right now in some places, though I
> >> >> > > can't
> >> >> > > find them in pg_dump. I know psql does that, see quote_ident().
> >> >> >
> >> >> > I think our general style rule is that identifiers embedded in
> >> >> > messages
> >> >> > are always double-quoted. There's an exception for type names, but
> >> >> > not otherwise. You're confusing the message case with printing SQL.
> >> >>
> >> >> OK. I was unclear if a status _display_ was a message like an error
> >> >> message.
> >> >>
> >> >
> >> > The attached patch fix missing double-quoted in "dumping contents of
> >> > table.." message and add schema name to other messages:
> >> > - "reading indexes for table \"%s\".\"%s\"\n"
> >> > - "reading foreign key constraints for table \"%s\".\"%s\"\n"
> >> > - "reading triggers for table \"%s\".\"%s\"\n"
> >> >
> >> > - "finding the columns and types of table \"%s\".\"%s\"\n"
> >> > - "finding default expressions of table \"%s\".\"%s\"\n"
> >> > - "finding check constraints for table \"%s\".\"%s\"\n"
> >> Cool additions. There may be a more elegant way to check if namespace
> >> is NULL, but I couldn't come up with one myself. So patch may be fine.
> >>
> >
> > Hi all,
> >
> > I think this small patch was lost. There are something wrong?
>
> Did it get added to a CommitFest?
>
> I don't see it there.
>
Given this is a very small and simple patch I thought it's not necessary...
Added to the next CommitFest.
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
Вложения
On Fri, Jul 25, 2014 at 4:45 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > > Given this is a very small and simple patch I thought it's not necessary... > > Added to the next CommitFest. I had a look at this patch, and here are a couple of comments: 1) Depending on how ArchiveEntry is called to register an object to dump, namespace may be NULL, but it is not the case namespace->dobj.name, so you could get the namespace name at the top of the function that have their verbose output improved with something like that: const char *namespace = tbinfo->dobj.namespace ? tbinfo->dobj.namespace->dobj.name : NULL; And then simplify the message output as follows: if (namespace) write_msg("blah \"%s\".\"%s\" blah", namespace, classname); else write_msg("blah \"%s\" blah", classname); You can as well safely remove the checks on namespace->dobj.name. 2) I don't think that this is correct: - ahlog(AH, 1, "processing data for table \"%s\"\n", - te->tag); + ahlog(AH, 1, "processing data for table \"%s\".\"%s\"\n", + AH->currSchema, te->tag); There are some code paths where AH->currSchema is set to NULL, and I think that you should use te->namespace instead. 3) Changing only this message is not enough. The following verbose messages need to be changed too for consistency: - pg_dump: creating $tag $object - pg_dump: setting owner and privileges for [blah] I have been pondering as well about doing similar modifications to the error message paths, but it did not seem worth it as this patch is aimed only for the verbose output. Btw, I have basically fixed those issues while doing the review, and finished with the attached patch. Fabrizio, is this new version fine for you? Regards, -- Michael
Вложения
On Wed, Aug 20, 2014 at 2:43 AM, Michael Paquier <michael.paquier@gmail.com> wrote:
>
> I had a look at this patch, and here are a couple of comments:
> 1) Depending on how ArchiveEntry is called to register an object to
> dump, namespace may be NULL, but it is not the case
> namespace->dobj.name, so you could get the namespace name at the top
> of the function that have their verbose output improved with something
> like that:
> const char *namespace = tbinfo->dobj.namespace ?
> tbinfo->dobj.namespace->dobj.name : NULL;
> And then simplify the message output as follows:
> if (namespace)
> write_msg("blah \"%s\".\"%s\" blah", namespace, classname);
> else
> write_msg("blah \"%s\" blah", classname);
> You can as well safely remove the checks on namespace->dobj.name.
Ok
> 2) I don't think that this is correct:
> - ahlog(AH, 1, "processing data
> for table \"%s\"\n",
> - te->tag);
> + ahlog(AH, 1, "processing data
> for table \"%s\".\"%s\"\n",
> + AH->currSchema, te->tag);
> There are some code paths where AH->currSchema is set to NULL, and I
> think that you should use te->namespace instead.
Yes, you are correct!
> 3) Changing only this message is not enough. The following verbose
> messages need to be changed too for consistency:
> - pg_dump: creating $tag $object
> - pg_dump: setting owner and privileges for [blah]
>
> I have been pondering as well about doing similar modifications to the
> error message paths, but it did not seem worth it as this patch is
> aimed only for the verbose output. Btw, I have basically fixed those
> issues while doing the review, and finished with the attached patch.
> Fabrizio, is this new version fine for you?
>
I just change "if (tbinfo->dobj.namespace != NULL)" to "if (tbinfo->dobj.namespace)".
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello
Вложения
On Thu, Aug 21, 2014 at 5:11 AM, Fabrízio de Royes Mello <fabriziomello@gmail.com> wrote: > I just change "if (tbinfo->dobj.namespace != NULL)" to "if > (tbinfo->dobj.namespace)". Fine for me. I am marking this patch as ready for committer. -- Michael
<div dir="ltr"><div class="gmail_extra"><br />On Wed, Aug 20, 2014 at 8:24 PM, Michael Paquier <<a href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>wrote:<br />><br />> On Thu, Aug 21, 2014at 5:11 AM, Fabrízio de Royes Mello<br /> > <<a href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>>wrote:<br />> > I just change "if (tbinfo->dobj.namespace!= NULL)" to "if<br />> > (tbinfo->dobj.namespace)".<br /> > Fine for me. I am markingthis patch as ready for committer.<br />><br /></div><div class="gmail_extra"><br />Thanks!<br /></div><div class="gmail_extra"><br/>--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br /> >> Timbira: <ahref="http://www.timbira.com.br">http://www.timbira.com.br</a><br />>> Blog: <a href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/> >> Twitter: <a href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>
On 08/20/2014 11:11 PM, Fabrízio de Royes Mello wrote: > On Wed, Aug 20, 2014 at 2:43 AM, Michael Paquier <michael.paquier@gmail.com> > wrote: >> >> I had a look at this patch, and here are a couple of comments: >> 1) Depending on how ArchiveEntry is called to register an object to >> dump, namespace may be NULL, but it is not the case >> namespace->dobj.name, so you could get the namespace name at the top >> of the function that have their verbose output improved with something >> like that: >> const char *namespace = tbinfo->dobj.namespace ? >> tbinfo->dobj.namespace->dobj.name : NULL; >> And then simplify the message output as follows: >> if (namespace) >> write_msg("blah \"%s\".\"%s\" blah", namespace, classname); >> else >> write_msg("blah \"%s\" blah", classname); >> You can as well safely remove the checks on namespace->dobj.name. > > Ok AFAICS, the namespace can never be NULL in any of these. There is a "selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name)" call before or after printing the message, so if tbinfo->dobj.namespace is NULL, you'll crash anyway. Please double-check, and remove the dead code if you agree. - Heikki
On Tue, Aug 26, 2014 at 3:48 AM, Heikki Linnakangas <hlinnakangas@vmware.com> wrote: > AFAICS, the namespace can never be NULL in any of these. There is a > "selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name)" call before or > after printing the message, so if tbinfo->dobj.namespace is NULL, you'll > crash anyway. Please double-check, and remove the dead code if you agree. Ah right, this field is used in many places. Even for pg_backup_archiver.c, the portion of code processing data always has the namespace set. I am sure that Fabrizio would have done that quickly, but as I was on this thread I simplified the patch as attached. Regards, -- Michael
Вложения
On 08/26/2014 10:10 AM, Michael Paquier wrote: > On Tue, Aug 26, 2014 at 3:48 AM, Heikki Linnakangas > <hlinnakangas@vmware.com> wrote: >> AFAICS, the namespace can never be NULL in any of these. There is a >> "selectSourceSchema(fout, tbinfo->dobj.namespace->dobj.name)" call before or >> after printing the message, so if tbinfo->dobj.namespace is NULL, you'll >> crash anyway. Please double-check, and remove the dead code if you agree. > Ah right, this field is used in many places. Even for > pg_backup_archiver.c, the portion of code processing data always has > the namespace set. I am sure that Fabrizio would have done that > quickly, but as I was on this thread I simplified the patch as Ok thanks, committed. - Heikki
<div dir="ltr"><div class="gmail_extra"><br />On Tue, Aug 26, 2014 at 4:10 AM, Michael Paquier <<a href="mailto:michael.paquier@gmail.com">michael.paquier@gmail.com</a>>wrote:<br />><br />> On Tue, Aug 26, 2014at 3:48 AM, Heikki Linnakangas<br /> > <<a href="mailto:hlinnakangas@vmware.com">hlinnakangas@vmware.com</a>>wrote:<br />> > AFAICS, the namespace can neverbe NULL in any of these. There is a<br />> > "selectSourceSchema(fout, tbinfo->dobj.namespace-><a href="http://dobj.name">dobj.name</a>)"call before or<br /> > > after printing the message, so if tbinfo->dobj.namespaceis NULL, you'll<br />> > crash anyway. Please double-check, and remove the dead code if youagree.<br />> Ah right, this field is used in many places. Even for<br /> > pg_backup_archiver.c, the portion ofcode processing data always has<br />> the namespace set. I am sure that Fabrizio would have done that<br />> quickly,but as I was on this thread I simplified the patch as<br />> attached.<br /> ><br /><br /></div><div class="gmail_extra">ThanksMichael... last night I was working on a refactoring in "tablecmds.c".<br /></div><div class="gmail_extra"><br/></div><div class="gmail_extra">Regards,<br /><br /></div><div class="gmail_extra">--<br />Fabríziode Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/> >> Linkedin: <a href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>