Thank you for the new patch. Sorry to have overlooked some
versions. I'm looking the v19 patch now.
make complains for an unused variable.
| syncrep.c: In function ‘SyncRepGetSyncStandbys’:
| syncrep.c:601:13: warning: variable ‘next’ set but not used
[-Wunused-but-set-variable]
| ListCell *next;
At Thu, 24 Mar 2016 22:29:01 +0900, Masahiko Sawada <sawada.m...@gmail.com>
wrote in <CAD21AoCxwezOTf9kLQRhuf2y=1c_fgjcormqjfqhomqw8eg...@mail.gmail.com>
> >> > SyncRepInitConfig()->SyncRepFreeConfig() has already pfree'd that
> >> > in the patch. Or am I missing something?
> >
> > Sorry, instead, the memory from strdup() will be abandoned in
> > upper level. (Thinking for some time..) Ah, I found that the
> > problem should be here.
> >
> > > SyncRepFreeConfig(SyncRepConfigData *config)
> > > {
> > ...
> > !> list_free(config->members);
> > > pfree(config);
> > > }
> >
> > The list_free *doesn't* free the memory blocks pointed by
> > lfirst(cell), which has been pstrdup'ed. It should be
> > list_free_deep(config->members) instead to free it completely.
Fujii> Yep, but SyncRepFreeConfig() already uses list_free_deep
Fujii> in the latest patch. Could you read the latest version
Fujii> that I posted upthread.
Sorry for overlooked the version. Every pair of parse(or
SyncRepUpdateConfig) and SyncRepFreeConfig is on the same memory
context so it seems safe (but might be fragile since it relies on
that the caller does so.).
> >> Previous(9.5 or before) s_s_names also accepts non-ASCII character and
> >> non-printable character, and can show it without replacing these
> >> character to '?'.
> >
> > Thank you for pointint it out (it was completely out of my
> > mind..). I have no objection to keep the previous behavior.
> >
> >> From backward compatibility perspective, we should not choose #1 or #2.
> >> Different behaviour between previous and current s_s_names is that
> >> previous s_s_names doesn't accept the node name having the sort of
> >> white-space character that isspace() returns true with.
> >> But current s_s_names allows us to specify such a node name.
> >> I guess that changing such behaviour is enough for fixing this issue.
> >> Thoughts?
> >
>
> Attached latest patch incorporating all review comments so far.
>
> Aside from the review comments, I did following changes;
> - Add logic to avoid fatal exit in yy_fatal_error().
Maybe good catch, but..
> syncrep_scanstr(const char *str)
..
> * Regain control after a fatal, internal flex error. It may have
> * corrupted parser state. Consequently, abandon the file, but trust
~~~~~~~~~~~~~~~~
> * that the state remains sane enough for syncrep_yy_delete_buffer().
~~~~~~~~~~~~~~~~~~~~~~~~
guc-file.l actually abandones the config file but syncrep_scanner
reads only a value of an item in it. And, the latter is
eventually true but a bit hard to understand.
The patch will emit a mysterious error message like this.
> invalid value for parameter "synchronous_standby_names": "2[a,b,c]"
> configuration file ".../postgresql.conf" contains errors
This is utterly wrong. A bit related to that, it seems to me that
syncrep_scan.l doesn't need the same mechanism with
guc-file.l. The nature of the modification would be making
call_*_check_hook to be tri-state instead of boolean. So just
cathing errors in call_*_check_hook and ereport()'ing as SQL
parser does seems enough, but either will do for me.
> - Improve regression test cases.
I forgot to mention that, but additionalORDER BY makes the test
robust.
I doubt the validity of the behavior in the following test.
> # Change the synchronous_standby_names = '2[standby1,*,standby2]' and check
> sync_state
Is this regarded as a correct as a value for it?
> Also I felt a sense of discomfort regarding using [ and ] as a special
> character for priority method.
> Because (, ) and [, ] are a little similar each other, so it would
> easily make many syntax errors when nested style is supported.
> And the synopsis of that in documentation is odd;
> synchronous_standby_names = 'N [ node_name [, ...] ]'
>
> This topic has been already discussed before but, we might want to
> change it to other characters such as < and >?
I don't mind ether but as Robert said, it is true that the
characters essentially to be used to enclose something should be
preferred to other characters. Distinguishability of glyphs has
less signinficance, perhaps.
# LISPers don't hesitate to dive into Sea of Parens.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
