[CBRD-26519] AUTO_INCREMENT 컬럼은 테이블 당 한개만 허용#6815
[CBRD-26519] AUTO_INCREMENT 컬럼은 테이블 당 한개만 허용#6815childyouth wants to merge 4 commits intoCUBRID:developfrom
Conversation
|
/run all |
src/query/execute_schema.c
Outdated
|
|
||
| if (error == NO_ERROR) | ||
| { | ||
| if (*attr == NULL) |
There was a problem hiding this comment.
*attr 전에 attr이 널이 아닌지 체크 되어야 합니다.
적어도 assert(attr != NULL); 이라도 넣어 주세요
그런데 만약 *attr이 널이 아니라면 어떻게 되는 건가요?
그런 일이 발생하면 않되는 것 아닌지요
즉 assert ( attr && *attr == NULL); 이런 조건은 반드시 지켜져야 한다... 뭐 이런 것은 아닌가요?
There was a problem hiding this comment.
안전하게 구동하기 위해 if 및 assert에 attr에 대한 검증을 포함하겠습니다.
감사합니다.
There was a problem hiding this comment.
Pull request overview
This PR enforces MySQL-compatible behavior by restricting tables to have only one AUTO_INCREMENT column. The implementation adds validation during CREATE TABLE and ALTER TABLE operations to ensure that no more than one AUTO_INCREMENT attribute exists per table.
Changes:
- Added new error code
ER_AUTO_INCREMENT_SINGLE_COL_ONLYwith corresponding error messages in English and Korean - Introduced
do_set_auto_incrementfunction to consolidate AUTO_INCREMENT validation and creation logic - Refactored
do_add_attributeanddo_change_att_schema_onlyto use the new validation function
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/base/error_code.h | Added new error code ER_AUTO_INCREMENT_SINGLE_COL_ONLY and updated ER_LAST_ERROR |
| msg/en_US.utf8/cubrid.msg | Added English error message for single AUTO_INCREMENT restriction |
| msg/ko_KR.utf8/cubrid.msg | Added Korean error message for single AUTO_INCREMENT restriction |
| src/query/execute_schema.c | Implemented validation logic through new do_set_auto_increment function and refactored existing code to use it |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: ctshim <78332782+ctshim@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| if (ai_serial != NULL) | ||
| { | ||
| /* we already found a serial. AMBIGUITY! */ | ||
| /* | ||
| * we already found a serial. AMBIGUITY! | ||
| * CUBRID 11.5 (guava) allows only one AUTO_INCREMENT attribute per class. | ||
| * Consider removing this logic. | ||
| */ | ||
| ERROR0 (error, ER_AUTO_INCREMENT_SINGLE_COL_AMBIGUITY); | ||
| goto change_ai_error; | ||
| } |
There was a problem hiding this comment.
해당 로직은 이 정책이 결정된 이번 PR에서 제거되는 것이 좋지 않을까요?
There was a problem hiding this comment.
자주 사용되지 않는 구문에서 오류체크 구문을 굳이 제거해야 할까 하여 남겨놓은 상태입니다.
(ALTER TABLE AUTO_INCREMENT = )
There was a problem hiding this comment.
의미가 없는 코드이고 스펙 변경이 확정되었다면 지우는 것이 좋다고 생각하는데, 회의 때 같이 이야기해보는 것이 좋을 거 같습니다.
| 1369 사용자 "%1$s"는 로그인할 수 없습니다. | ||
|
|
||
| 1370 마지막 에러 | ||
| 1370 AUTO_INCREMENT인 애트리뷰트가 존재합니다. 테이블은 한 개의 AUTO_INCREMENT만 가질 수 있습니다. |
There was a problem hiding this comment.
해당 문구는 생성 시에만 해당되는 걸까요?
만약 그렇다면 테이블 생성 전이기 때문에 아래처럼 작성하는 것이 더 명확하지 않을까요?
| 1370 AUTO_INCREMENT인 애트리뷰트가 존재합니다. 테이블은 한 개의 AUTO_INCREMENT만 가질 수 있습니다. | |
| 1370 테이블은 한 개의 AUTO_INCREMENT만 가질 수 있습니다. |
There was a problem hiding this comment.
- CREATE 시 두 개의 AI가 들어가는 경우
- ALTER ADD COLUMN 으로 두 개의 AI를 생성하는 경우
- 이미 AI가 존재하는 상황에서 ALTER ADD/MODIFY/CHANGE 로 AI를 추가하는 경우
모든 경우에서 사용되는 문구입니다.
영문도 함께 제안사항으로 변경하도록 하겠습니다. 감사합니다.
There was a problem hiding this comment.
제 제안은 1,2번의 경우이고,
3번의 경우는 @childyouth 님이 사용하신 문구가 더 적절해 보이네요.
| MOP auto_increment_obj = NULL; | ||
| error = do_create_auto_increment_serial (parser, &auto_increment_obj, ctemplate->name, attribute); | ||
|
|
||
|
|
| if (attr != NULL) | ||
| { | ||
| att = *attr; | ||
| } |
There was a problem hiding this comment.
assert (attr != NULL) (5002 번째 줄) 에서 미리 체크해줘서 불필요 하지 않나요?
또는 현재 런타임에서 NULL이 들어오는 상황이 있는 걸까요?
There was a problem hiding this comment.
현재 런타임에서 SM_ATTRIBUTE** attr이 NULL로 들어오는 상황은 없습니다.
안전을 위해 assert와 if 둘 다 넣었는데 if는 제거하는 편이 나을까요?
There was a problem hiding this comment.
do_set_auto_increment() 를 호출하는 두 군데를 봐도 attr 이 null 이 될 수는 없습니다.
attr 의 null 검사 및 null 일 때 로직은 빼도 좋을 것 같습니다.
There was a problem hiding this comment.
저의 생각은 assert는 이후 해당 함수를 사용하는 개발자를 위해 남겨 놓아도 괜찮겠지만, if는 제거하는 것이 좋을 거 같습니다.
| SM_ATTRIBUTE ** attr) | ||
| { | ||
| SM_ATTRIBUTE *ctmpl_attrs = ctemplate->attributes; | ||
| SM_ATTRIBUTE *att = NULL; |
There was a problem hiding this comment.
attr를 직접 사용하는 것이 아닌 att 를 통해 간접적으로 사용하는 이유가 뭘까요?
There was a problem hiding this comment.
SM_ATTRIBUTE ** attr이 NULL인 경우 *attr 로 접근하는것이 위험하여 로컬 변수에서 작업한 뒤 return이 필요한 경우 넘겨주는 방식으로 구현했습니다.
assert와 if로 attr의 NULL 체크하는 부분을 모두 제거하는편이 좋을까요?
There was a problem hiding this comment.
assert 구문으로 attr의 NULL 체크를 하기 때문에 괜찮을 거라 생각합니다.
http://jira.cubrid.org/browse/CBRD-26519
Purpose
ALTER TABLE <TBL> AUTO_INCREMENT =구문 역시 한 개만 존재함을 가정하여 구현됨Implementation
Remarks