Performance Enhancement/Fix for Array Utility Functions
in [PgSql]
|
Prev: [HACKERS] CommitFest FAQ (was: dividing money by money)
Next: [BUGS] BUG #5487: dblink failed with 63 bytes connectionnames
From: Daniel Farina on 1 Jun 2010 20:32 On Wed, Mar 31, 2010 at 9:47 AM, Mike Lewis <mikelikespie(a)gmail.com> wrote: > Thanks. Added it. > > https://commitfest.postgresql.org/action/patch_view?id=292 I have reviewed this patch; this is my review: Regression tests pass with assertions enabled. Performance gains reported by author confirmed. The existence and naming of ARR_MAX_HEADER_SIZE is somewhat dubious, as it is: * Used in exactly one place (not necessarily a reason why it should not be reified into a stand-alone definition, though, but something to consider) * The array header refers to the NULL bitmap as well, but the interpretation used by the patch does not. I think this patch is safe, as all the array fields required are before the null bitmap, but I think the naming of this definition is very misleading. Generally I think the delimited untoasting of metadata from arrays separately from the payload is Not A Bad Idea. fdr -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Mike Lewis on 16 Jun 2010 02:40
> > > The existence and naming of ARR_MAX_HEADER_SIZE is somewhat dubious, > as it is: > Thanks you for the feedback. I cleaned up the patch. > * Used in exactly one place (not necessarily a reason why it should > not be reified into a stand-alone definition, though, but > something to consider) > Moved it to one definition > * The array header refers to the NULL bitmap as well, but the > interpretation used by the patch does not. > I renamed the macros to have NONULL in the name (hopefully it doesn't make them too long). I also added a comment. Not quite sure if it's the appropriate format, but I didn't feel it warranted 3 lines. Thanks, Mike Lewis |