On Thu, Jan 7, 2016 at 3:36 AM, Kyotaro HORIGUCHI
<horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> - 0001-Prepare-for-sharing-psqlscan-with-pgbench.patch
>
> This diff looks a bit large but most of them is cut'n-paste
> work and the substantial change is rather small.
>
> This refactors psqlscan.l into two .l files. The additional
> psqlscan_slash.l is a bit tricky in the point that recreating
> scan_state on transition between psqlscan.l.
I've looked at this patch a few times now but find it rather hard to
verify. I am wondering if you would be willing to separate 0001 into
subpatches. For example, maybe there could be one or two patches that
ONLY move code around and then one or more patches that make the
changes to that code. Right now, for example, psql_scan_setup() is
getting three additional arguments, but it's also being moved to a
different file. Perhaps those two things could be done one at a time.
I also think this patch could really benefit from a detailed set of
submission notes that specifically lay out why each change was made
and why. For instance, I see that psqlscan.l used yyless() while
psqlscanbody.l uses a new my_yyless() you've defined. There is
probably a great reason for that and I'm sure if I stare at this for
long enough I can figure out what that reason is, but it would be
better if you had a list of bullet points explaining what was changed
and why.
I would really like to see this patch committed; my problem is that I
don't have enough braincells to be sure that it's correct in the
present form.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company