Re: pageinspect patch, for showing tuple data - Mailing list pgsql-hackers

В письме от 4 сентября 2015 14:58:29 пользователь Michael Paquier написал:

> Yeah, I think that's acceptable to have a switch, defaulting to ERROR if
> caller specifies nothing.
>
> Here are some observations after looking at the code, no functional testing.
>
> +       int             error_mode = PG_NARGS()==3 ? PG_GETARG_BOOL(2)
>
> :NULL;
>
> When handling additional arguments, it seems to me that it is more readable
> to use something like that:
> if (PG_NARGS >= 3)
> {
>      arg = PG_GETARG_XXX(2);
>      //etc.
> }
>
> +       error_mode = error_mode?WARNING:ERROR;
>
> This generates warnings at compilation.
>
yeah, what I have done here is too complex with no reason. I've simplified this
code now.

> +       if (SRF_IS_FIRSTCALL() && (error_mode == WARNING))
> +       {
> +               ereport(WARNING,
> +                       (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +                               (errmsg("Runnung heap_page_item_attrs in
> WARNING mode."))));
> +       }
>
> I am not convinced that this is necessary.
I've removed it.

>
> +               inter_call_data->raw_page_size = VARSIZE(raw_page) -
> VARHDRSZ;
> +               if (inter_call_data->raw_page_size < SizeOfPageHeaderData)
> Including raw_page_size in the status data does not seem necessary to me.
> The page data is already available for each loop so you could just extract
> it from it.
>
> +               ereport(inter_call_data->error_level,
> +                               (errcode(ERRCODE_DATA_CORRUPTED),
> +                                       errmsg("Data corruption: number of
> attributes in tuple header is greater than number of attributes in tuple
> descripor")));
> I'd rather switch the error reports related to data corruption from ereport
> to elog, which is more suited for internal errors, and it seems to me that
> it is the case here.
Hm... First, since we have proper error code, do not see why not give it.

Second, this is not exactly internal error, in some cases user tries to parse
corrupted data on purpose. So for user it is not an internal error, error from
deep below, but error on the level he is currently working at. So I would not
call it internal error.

> heapfuncs.c:370:7: warning: variable 'raw_attr' is used uninitialized
> whenever 'if' condition is false [-Wsometimes-uninitialized]
>                 if (!is_null)
>                     ^~~~~~~~
> heapfuncs.c:419:43: note: uninitialized use occurs here
>                 raw_attrs = accumArrayResult(raw_attrs, raw_attr, is_null,
> BYTEAOID, fctx->multi_call_memory_ctx);
>                                                         ^~~~~~~~
> heapfuncs.c:370:3: note: remove the 'if' if its condition is always true
>                 if (!is_null)
>                 ^~~~~~~~~~~~~
> heapfuncs.c:357:17: note: initialize the variable 'raw_attr' to silence
> this warning
>                 Datum raw_attr;
> My compiler complains about uninitialized variables.
Fixed it

> +--
> +-- _heap_page_items()
> +--
> +CREATE FUNCTION _heap_page_items(IN page bytea, IN relname text,
> I am not sure why this is necessary and visibly it overlaps with the
> existing declaration of heap_page_items. The last argument is different
> though, and I recall that we decided not to use anymore the relation name
> specified as text, but its OID instead in this module.
Oh! This should not go to the final patch :-( Sorry. Removed it.


> Documentation is missing, that would be good to have to understand what
> each function is intended to do.

I were going to add documentation when this patch is commited, or at least
approved for commit. So I would know for sure that function definition would
not change, so had not to rewrite it again and again.

So if it is possible I would write documentation and test when this patch is
already approved.

> Code has some whitespaces.
I've found and removed some. Hope that was all of them...


--
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company