Re: ExecutorCheckPerms() hook
От | Stephen Frost |
---|---|
Тема | Re: ExecutorCheckPerms() hook |
Дата | |
Msg-id | 20100525124420.GO21875@tamriel.snowman.net обсуждение исходный текст |
Ответ на | Re: ExecutorCheckPerms() hook (KaiGai Kohei <kaigai@ak.jp.nec.com>) |
Ответы |
Re: ExecutorCheckPerms() hook
(KaiGai Kohei <kaigai@ak.jp.nec.com>)
|
Список | pgsql-hackers |
KaiGai, * KaiGai Kohei (kaigai@ak.jp.nec.com) wrote: > OK, the attached patch reworks it according to the way. Reviewing this patch, there are a whole slew of problems. #1: REALLY BIG ISSUE- Insufficient comment updates. You've changed function definitions in a pretty serious way as well as moved some code around such that some of the previous comments don't make sense. You have got to update comments when you're writing a patch. Indeed, the places I see a changes in comments are when you've removed what appears to still be valid and appropriate comments, or places where you've added comments which are just blatently wrong with the submitted patch. #2: REALLY BIG ISSUE- You've added ExecutorCheckPerms_hook as part of this patch- don't, we're in feature-freeze right now and should not be adding hooks at this time. #3: You didn't move ExecCheckRTPerms() and ExecCheckRTEPerms() to utils/acl and instead added executor/executor.h to rt_triggers.c. I don't particularly like that. I admit that DoCopy() already knew about the executor, and if that were the only case outside of the executor where ExecCheckRTPerms() was getting called it'd probably be alright, but we already have another place that wants to use it, so let's move it to a more appropriate place. #4: As mentioned previously, the hook (which should be added in a separate patch anyway) makes more sense to me to be in ExecCheckRTPerms(), not ExecCheckRTEPerms(). This also means that we need to be calling ExecCheckRTPerms() from DoCopy and RI_Initial_Check(), to make sure that the hook gets called. To that end, I wouldn't even expose ExecCheckRTEPerms() outside of acl.c. Also, there should be a big comment about not using or calling ExecCheckRTEPerms() directly outside of ExecCheckRTPerms() since the hook would then be skipped. #5: In DoCopy, you can remove relPerms and remainingPerms, but I'd probably leave required_access up near the top and then just use it to set rte->required_access directly rather than moving that bit deep down into the function. #6: I havn't checked yet, but if there are other things in an RTE which would make sense in the DoCopy case, beyond just what's needed for the permissions checking, and which wouldn't be 'correct' with a NULL'd value, I would set those. Yes, we're building the RTE to check permissions, but we don't want someone downstream to be suprised when they make a change to something in the permissions checking and discover that a value in RTE they expected to be there wasn't valid. Even more so, if there are function helpers which can be used to build an RTE, we should be using them. The same goes for RI_Initial_Check(). #7: I'd move the conditional if (is_from) into the foreach which is building the columnsSet and eliminate the need for columnsSet; I don't see that it's really adding much here. #8: When moving ExecCheckRTPerms(), you should rename it to be more like the other function calls in acl.h Perhaps pg_rangetbl_aclcheck()? Also, it should return an actual AclResult instead of just true/false. Thanks, Stephen
В списке pgsql-hackers по дате отправления:
Следующее
От: Alex GoncharovДата:
Сообщение: Re: libpq, PQexecPrepared, data size sent to FE vs. FETCH_COUNT