2014-03-23 15:14 GMT+01:00 Pavel Stehule <pavel.steh...@gmail.com>: > Review shadow_v6 patch > > Hello > > I did a recheck a newest version of this patch: > > 1. There is a wide agreement on implemented feature - nothing changed from > previous review - it is not necessary comment it again. > > 2. v6 patch: patching cleanly, compilation without errors and warnings, > all regress tests passed > > Tom's objections was related to GUC part. It is redesigned as Tom proposed. > > The code is good - and I don't see any problem there. > > I have only one objection - What I remember - more usual is using a list > instead a bitmap for these purposes - typical is DefElem struct. Isn't it > better? >
A using DefElem will be longer, but it is typical pattern for this case in Postgres. What is opinion of other hackers? Pavel > > Regards > > Pavel > > > 2014-03-20 12:39 GMT+01:00 Petr Jelinek <p...@2ndquadrant.com>: > > On 20/03/14 00:32, Tom Lane wrote: >> >>> >>> TBH, if I thought this specific warning was the only one that would ever >>> be there, I'd probably be arguing to reject this patch altogether. >>> >> >> Of course, nobody assumes that it will be the only one. >> >> >> >>> Also, adding GUC_LIST_INPUT later is not really cool since it changes >>> the parsing behavior for the GUC. If it's going to be a list, it should >>> be one from day zero. >>> >>> >> Actually it does not since it all has to be handled in check/assign hook >> anyway. >> >> But nevertheless, I made V6 with doc change suggested by Alvaro and also >> added this list handling framework for the GUC params. >> In the end it is probably less confusing now that the implementation uses >> bitmask instead of bool when the user facing functionality talks about >> list... >> >> This obviously needs code review again (I haven't changed tests since >> nothing changed from user perspective). >> >> >> >> -- >> Petr Jelinek http://www.2ndQuadrant.com/ >> PostgreSQL Development, 24x7 Support, Training & Services >> > >
