Alvaro, Thanks for the review.
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Note the first comment in this hunk was not update to talk about NULL
> instead of "":
Ah, good catch, will fix.
> Hm, this is a bit odd. I thought you were going to return the subset of
> columns that the user had permission to read, not empty if any of them
> was inaccesible. Did I misunderstand?
The subset is for regular relations. For indexes and keys, we only
return either the whole key or nothing. Returning a partial key didn't
make much sense to me and we also don't know which columns were actually
provided by the user since it's going through the index AM, so we can't
return just what the user provided.
> > +#define GetModifiedColumns(relinfo, estate) \
> > + (rt_fetch((relinfo)->ri_RangeTableIndex,
> > (estate)->es_range_table)->modifiedCols)
>
> I assume you are aware that this GetModifiedColumns() macro is a
> duplicate of another one found elsewhere. However I think this is not
> such a hot idea; the UPSERT patch series has a preparatory patch that
> changes that other macro definition, as far as I recall; probably it'd
> be a good idea to move it elsewhere, to avoid a future divergence.
Yeah, I had meant to do something about that and had looked around but
didn't find any particularly good place to put it. Any suggestions on
that?
> > @@ -1689,25 +1722,54 @@ ExecWithCheckOptions(ResultRelInfo *resultRelInfo,
> > * dropped columns. We used to use the slot's tuple descriptor to decode
> > the
> > * data, but the slot's descriptor doesn't identify dropped columns, so we
> > * now need to be passed the relation's descriptor.
> > + *
> > + * Note that, like BuildIndexValueDescription, if the user does not have
> > + * permission to view any of the columns involved, a NULL is returned.
> > Unlike
> > + * BuildIndexValueDescription, if the user has access to view a subset of
> > the
> > + * column involved, that subset will be returned with a key identifying
> > which
> > + * columns they are.
> > */
>
> Ah, I now see that you are aware of the NULL-or-nothing nature of
> BuildIndexValueDescription ... but the comments there don't explain the
> reason. Perhaps a comment in BuildIndexValueDescription is in order
> rather than a code change?
Yeah, I'll add comments to BuildIndexValueDescription to explain why
it's all-or-nothing.
I've also been working on back-patching the fixes; the next update will
hopefully include patches for all branches.
Thanks!
Stephen
signature.asc
Description: Digital signature
