New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[15721] Sequence second PR #1345

Closed
wants to merge 37 commits into
base: master
from

Conversation

Projects
None yet
8 participants
@bdengNE

bdengNE commented May 5, 2018

In the second PR, we have fixed the problems found in first PR and modified codes based on the updates in the master branch. In addition, we are currently adding features such as drop sequence and currency control, etc (still implementing them and you will see more commits to the PR very soon)

HenryZhou0333 and others added some commits Mar 22, 2018

danae-s and others added some commits May 5, 2018

}
/*@brief evict sequence catalog object from cache
* @param sequence_name, database_oid

This comment has been minimized.

@lym953

lym953 May 11, 2018

Each parameter should be put on a separate line.

}
/*@brief get sequence catalog object from cache
* @param sequence_name, database_oid

This comment has been minimized.

@lym953

lym953 May 11, 2018

Each parameter should be put on a separate line.

}
/*@brief get the hash key given the sequence information
* @param sequence_name, database_oid

This comment has been minimized.

@lym953

lym953 May 11, 2018

Each parameter should be put on a separate line.

}
/* @brief Delete the sequence by name.
* @param database_oid the databse_oid associated with the sequence

This comment has been minimized.

@lym953

lym953 May 11, 2018

Wrong parameter in doc

// the result is a vector of executor::LogicalTile
auto result_tiles =
GetResultWithIndexScan(column_ids, index_offset, values, txn);
// carefull! the result tile could be null!

This comment has been minimized.

@lym953

lym953 May 11, 2018

Typo: carefull

* @return the result of executing CurrVal
*/
type::Value SequenceFunctions::_Currval(const std::vector<type::Value> &args) {
executor::ExecutorContext* ctx=(executor::ExecutorContext*)args[1].GetAs<uint64_t>();

This comment has been minimized.

@lym953

lym953 May 11, 2018

Not formatted

}
/*@brief The wrapper function to get the current value for the specified sequence
* @param sequence name, executor context

This comment has been minimized.

@lym953
if (seq_min > seq_max) {
throw SequenceException(
StringUtil::Format(
"MINVALUE (%d) must be less than MAXVALUE (%d)", seq_min, seq_max));

This comment has been minimized.

@lym953

lym953 May 11, 2018

less than -> less than or equal to
or: cannot be greater than

@@ -61,7 +61,8 @@ class PlanExecutor {
const std::vector<type::Value> &params,
const std::vector<int> &result_format,
std::function<void(executor::ExecutionResult,
std::vector<ResultValue> &&)> on_complete);
std::vector<ResultValue> &&)> on_complete,
std::string default_database_name="");

This comment has been minimized.

@lym953

lym953 May 11, 2018

Not formatted

@@ -730,6 +730,16 @@ typedef struct CreateSchemaStmt
bool if_not_exists; /* just do nothing if schema already exists? */
} CreateSchemaStmt;
typedef struct CreateSeqStmt
{
NodeTag type;

This comment has been minimized.

@lym953

lym953 May 11, 2018

Not formatted

// check if already in cache
if (sequence_objects_cache.find(hash_key) !=
sequence_objects_cache.end()) {

This comment has been minimized.

@yjc408

yjc408 May 11, 2018

Do you want to throw an exception for this case?

int64_t result = seq_curr_val;
seq_prev_val = result;
if (seq_increment > 0) {
if ((seq_max >= 0 && seq_curr_val > seq_max - seq_increment) ||

This comment has been minimized.

@yjc408

yjc408 May 11, 2018

Could you add some comments to explain the logic?

std::unique_ptr<storage::Tuple> tuple(
new storage::Tuple(catalog_table_->GetSchema(), true));
auto val0 = type::ValueFactory::GetIntegerValue(GetNextOid());

This comment has been minimized.

@yjc408

yjc408 May 11, 2018

More meaningful variable names here

return new_sequence;
}
bool SequenceCatalog::UpdateNextVal(oid_t sequence_oid, int64_t nextval,

This comment has been minimized.

@yjc408

yjc408 May 11, 2018

Missing doc here

LOG_INFO("sequence %d will be deleted!", sequence_oid);
oid_t database_oid = database_object->GetDatabaseOid();
DeleteSequenceByName(sequence_name, database_oid, txn);

This comment has been minimized.

@yjc408

yjc408 May 11, 2018

Do you want to get the return value of this function and return fail sometimes?

@@ -43,5 +47,9 @@ type::EphemeralPool *ExecutorContext::GetPool() {
return pool_.get();
}
std::string ExecutorContext::GetDatabaseName() const {

This comment has been minimized.

@yjc408

yjc408 May 11, 2018

usually this function is set in header file.

This comment has been minimized.

@danae-s

danae-s May 13, 2018

This is what other functions in the same file do, so gonna keep it like this.

ret = txn_manager.CommitTransaction(mini_txn);
}
return val;
} else {

This comment has been minimized.

@yjc408

yjc408 May 11, 2018

Don't need this else

This comment has been minimized.

@danae-s

danae-s May 13, 2018

We do, for the case when no such sequence exists at all.

@@ -44,6 +45,8 @@ class ExecutorContext {
type::EphemeralPool *GetPool();
std::string GetDatabaseName() const;

This comment has been minimized.

@yjc408

yjc408 May 11, 2018

Like previous comment, you can make this function inline to return the database name (like what other files do)

This comment has been minimized.

@danae-s

danae-s May 13, 2018

This is what other functions in the same file do, so gonna keep it like this.

@@ -616,6 +616,7 @@ enum class CreateType {
CONSTRAINT = 4, // constraint create type
TRIGGER = 5, // trigger create type
SCHEMA = 6, // schema create type
SEQUENCE = 7 // sequencce create type

This comment has been minimized.

@yjc408

yjc408 May 11, 2018

typo in comment

/* @brief get sequence oid from pg_sequence table given sequence_name and
* database_oid
* @param database_oid the databse_oid associated with the sequence

This comment has been minimized.

@yjc408

yjc408 May 11, 2018

wrong param order

bool CatalogCache::InsertSequenceObject(
std::shared_ptr<SequenceCatalogObject> sequence_object) {
if (!sequence_object || sequence_object->seq_oid == INVALID_OID) {
return false; // invalid object

This comment has been minimized.

@yangjuns

yangjuns May 11, 2018

Member

In this case, false doesn't indicate the sequence exists in the cache. Probably need to change the @return spec or requires the sequence_object to be valid.

std::size_t hash_key = GetHashKey(sequence_name, database_oid);
auto it = sequence_objects_cache.find(hash_key);
if (it == sequence_objects_cache.end()) {
return nullptr;

This comment has been minimized.

@yangjuns

yangjuns May 11, 2018

Member

It said it return object with invalid oid if not found. To make things consistent, instead of returning nullptr, we should probably return an SequenceCatalogObject with seq_oid = INVALID_OID. Or another easier way is to change the spec.

This comment has been minimized.

@danae-s

danae-s May 11, 2018

This is a consistent behavior with the existing methods (like GetTableObejct, etc.) in catalog_cache.cpp so I would like to keep it.

* @param sequence_name, database_oid
* @return hash key
*/
std::size_t CatalogCache::GetHashKey(const std::string sequence_name,

This comment has been minimized.

@yangjuns

yangjuns May 11, 2018

Member

Not sure but wouldn't using reference, const std::string &, avoid unnecessary copies of strings?

LOG_INFO("sequence %d will be deleted!", sequence_oid);
oid_t database_oid = database_object->GetDatabaseOid();

This comment has been minimized.

@yangjuns

yangjuns May 11, 2018

Member

Seems like you called GetDataBaseOid() three times. You could instead store it.

concurrency::TransactionContext *txn) {
oid_t index_offset = IndexId::DBOID_SEQNAME_KEY;
std::vector<type::Value> values;
values.push_back(type::ValueFactory::GetIntegerValue(database_oid).Copy());

This comment has been minimized.

@yangjuns

yangjuns May 11, 2018

Member

Why do we need Copy() here?

This comment has been minimized.

@HenryZhou0333

HenryZhou0333 May 14, 2018

Just following conventions from other files.

std::vector<type::Value> child_values;
PELOTON_ASSERT(func_.impl != nullptr);
for (auto &child : children_) {
child_values.push_back(child->Evaluate(tuple1, tuple2, context));
}
if (func_name_ == "nextval" || func_name_ == "currval") {
uint64_t ctx = (uint64_t)context;

This comment has been minimized.

@yangjuns

yangjuns May 11, 2018

Member

Casting a pointer to a 64bit integer and passing as an BigIntValue doesn't look like a good design for me.

->GetSystemCatalogs(database_oid)
->GetSequenceCatalog()
->GetSequence(database_oid, sequence_name, mini_txn);
if (sequence_object != nullptr) {

This comment has been minimized.

@yangjuns

yangjuns May 11, 2018

Member

The function EvictSequenceObject() grantees to evict the given database_oid. So I think you don't need to search for sequence_object and double check again. Or, since EvictSequenceObject() will return false and true, you can use this information for help you make decision.

This comment has been minimized.

@danae-s

danae-s May 13, 2018

The sequence may exist in the catalog but not exist in the cache. In this case, EvictSequenceObject() will return false but we want to call nextval on the sequence.

apavlo added a commit to apavlo/peloton that referenced this pull request Jun 10, 2018

apavlo added a commit to apavlo/peloton that referenced this pull request Jun 11, 2018

apavlo added a commit to apavlo/peloton that referenced this pull request Jun 11, 2018

@apavlo

This comment has been minimized.

Member

apavlo commented Jun 11, 2018

I am closing this PR. I will merge the sequence code manually without the temporary table patches.

@apavlo apavlo closed this Jun 11, 2018

@apavlo apavlo referenced this pull request Jun 14, 2018

Open

Sequences #1407

apavlo added a commit to apavlo/peloton that referenced this pull request Jun 19, 2018

Initial checkin with the sequence code (without temporary tables) from
…cmu-db#1345

Checking in the testing files from cmu-db#1345

These should be the last files that we need from from cmu-db#1345

This compiles and passes all the tests except for the sequence test (obviously)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment