[15721] Implement Sequence in peloton #1292

HenryZhou0333 · 2018-04-12T18:36:04Z

This pull request contains the implementation of create sequence.

Right now we are treating sequence as normal transaction as Andy suggested in the project status update meeting.

In this pull request, we modified postgresparser.cpp to support CREATE SEQUENCE statement, with options as defined in pg_sequence(10.0). We currently don't support AS(the third-party pg_query table is not updated yet) and OWNED BY(which we plan to discuss with the other team for namespace issue).

We also created a new file in the catalog, called sequence_catalog.cpp , that can deal with insert, get and delete. In the same file, the SequenceCatalogObject class embeds the functionality of the sequence object.

…r.cpp

coveralls · 2018-04-12T20:00:36Z

Coverage decreased (-0.03%) to 76.935% when pulling 6897566 on HenryZhou0333:master into a9d83a0 on cmu-db:master.

…functioning well

yjc408 · 2018-04-18T23:03:52Z

src/catalog/catalog.cpp

@@ -148,12 +150,13 @@ void Catalog::Bootstrap() {
  DatabaseMetricsCatalog::GetInstance(txn);
  TableMetricsCatalog::GetInstance(txn);
  IndexMetricsCatalog::GetInstance(txn);
-  QueryMetricsCatalog::GetInstance(txn);  
+  QueryMetricsCatalog::GetInstance(txn);


It might be better to keep spaces and lines to be consistent as before.

yjc408 · 2018-04-18T23:10:34Z

src/catalog/sequence_catalog.cpp

+
+SequenceCatalog::~SequenceCatalog() {}
+
+/* @brief   Delete the sequence by name.


Inconsistent brief here.

yjc408 · 2018-04-18T23:18:39Z

src/catalog/sequence_catalog.cpp

+ * limit.
+ */
+int64_t SequenceCatalogObject::get_next_val() {
+  int64_t result = seq_curr_val;


Could you add more comments here?

yjc408 · 2018-04-18T23:22:49Z

src/executor/create_executor.cpp

+              ResultTypeToString(txn->GetResult()).c_str());
+  }
+
+  return (true);


Why it always returns true here?

yjc408 · 2018-04-18T23:34:25Z

src/planner/create_plan.cpp

+      create_type = CreateType::SEQUENCE;
+      database_name = std::string(parse_tree->GetDatabaseName());
+
+      sequence_name = parse_tree->sequence_name;


Do you still need to assign default values for fields in createStatement if you always assign values to them explictly here?

The value could be empty in the postgres, which leave some field undefined.

lym953 · 2018-04-18T23:43:07Z

test/parser/postgresparser_test.cpp

+      parser.BuildParseTree(query).release());
+  EXPECT_TRUE(stmt_list->is_valid);
+  if (!stmt_list->is_valid) {
+    LOG_ERROR("Message: %s, line: %d, col: %d", stmt_list->parser_msg,


This seems to be dead code.

yjc408 · 2018-04-19T00:12:26Z

src/include/catalog/sequence_catalog.h

+  bool seq_cycle;         // Whether the sequence cycles
+  concurrency::TransactionContext *txn_;
+
+  std::mutex sequence_mutex;  // mutex for all operations


Maybe unnecessary to have mutex for the current implementation.

Is this mutex being used anywhere? Also, you should use common::synchronization::DirtyMutexLatch

https://github.com/cmu-db/peloton/blob/master/src/include/common/synchronization/mutex_latch.h

yjc408 · 2018-04-19T00:23:28Z

src/expression/function_expression.cpp

  std::vector<type::Value> child_values;

  PELOTON_ASSERT(func_.impl != nullptr);
  for (auto &child : children_) {
    child_values.push_back(child->Evaluate(tuple1, tuple2, context));
  }
+  uint64_t ctx = (uint64_t)context;


Already included in ctx. not necessary to cast and pass.

Actually not included, so need to pass it explicitly for sequence functions.

yangjuns

Private functions should be CamelCase; Nextval() Currval() should be in a separate files; I think there are fundamental design problems related to default_database_name.

yangjuns · 2018-04-18T23:48:03Z

src/catalog/abstract_catalog.cpp

@@ -272,5 +274,69 @@ void AbstractCatalog::AddIndex(const std::vector<oid_t> &key_attrs,
            index_name.c_str(), (int)catalog_table_->GetOid());
 }

+/*@brief   Update specific columns using index scan


/*(space)brief
is probably better

yangjuns · 2018-04-18T23:48:44Z

src/catalog/sequence_catalog.cpp

+ * @exception throws SequenceException if the sequence exceeds the upper/lower
+ * limit.
+ */
+int64_t SequenceCatalogObject::get_next_val() {


I think function names should be Camel case, even for private functions.

Yes! Please fix!

yangjuns · 2018-04-18T23:49:46Z

src/include/executor/executor_context.h

@@ -30,7 +30,8 @@ namespace executor {
 class ExecutorContext {
 public:
  explicit ExecutorContext(concurrency::TransactionContext *transaction,
-                           codegen::QueryParameters parameters = {});
+                           codegen::QueryParameters parameters = {},
+                           std::string default_database_name = "");


Is this appropriate to add default_database_name here? Even if you want to know the default_database_name when call string_functions, this shouldn't be a separate argument.

I think we need this but the default value for this parameter should not be an empty string. That's going to cause problems later on.

yangjuns · 2018-04-19T00:17:02Z

src/include/function/old_engine_string_functions.h

@@ -68,6 +68,10 @@ class OldEngineStringFunctions {
  // Upper, Lower
  static type::Value Upper(const std::vector<type::Value> &args);
  static type::Value Lower(const std::vector<type::Value> &args);
+
+  // Sequence-related
+  static type::Value Nextval(const std::vector<type::Value> &args);


These two functions, Nextval() and Currval() should be put into a new sequence file and update binder to bind the file

Correct! These are not string functions!

yangjuns · 2018-04-19T00:31:41Z

src/include/function/string_functions.h

@@ -74,6 +74,12 @@ class StringFunctions {
  // Length will return the number of characters in the given string
  static uint32_t Length(executor::ExecutorContext &ctx, const char *str,
                         uint32_t length);
+
+  // Nextval will return the next value of the given sequence
+  static uint32_t Nextval(executor::ExecutorContext &ctx, const char *sequence_name);


These two functions, Nextval() and Currval() should be put into a new sequence file and update binder to bind the file.

Andy suggests to merge the two files as other catalogs.

yangjuns · 2018-04-19T00:32:44Z

src/include/parser/postgresparser.h


  // transform helper for subquery expressions
  static expression::AbstractExpression *SubqueryExprTransform(SubLink *node);

+  static void parse_sequence_params(List *options,


Functions names should be Camel case.

yangjuns · 2018-04-19T00:43:25Z

src/include/function/string_functions.h

@@ -74,6 +74,12 @@ class StringFunctions {
  // Length will return the number of characters in the given string
  static uint32_t Length(executor::ExecutorContext &ctx, const char *str,
                         uint32_t length);
+
+  // Nextval will return the next value of the given sequence
+  static uint32_t Nextval(executor::ExecutorContext &ctx, const char *sequence_name);


These two functions don't seem to be tested.

yangjuns · 2018-04-19T00:53:10Z

src/include/catalog/sequence_catalog.h

+#include "catalog/abstract_catalog.h"
+#include "catalog/catalog_defaults.h"
+
+#define SEQUENCE_CATALOG_NAME "pg_sequence"


I think this macro should be declared in catalog_defaults.h.

lym953 · 2018-04-19T00:23:42Z

src/catalog/sequence_catalog.cpp

+                                         const std::string &sequence_name,
+                                         concurrency::TransactionContext *txn) {
+  if (txn == nullptr) {
+    LOG_TRACE("Do not have transaction to drop sequence: %s",


Logging level of LOG_TRACE may be too low here. May consider switching to a higher level, such as LOG_INFO.

This should throw a CatalogException like the other catalog classes.

lym953 · 2018-04-19T00:24:13Z

src/catalog/sequence_catalog.cpp

+  oid_t sequence_oid = SequenceCatalog::GetInstance().GetSequenceOid(
+      sequence_name, database_object->GetDatabaseOid(), txn);
+  if (sequence_oid == INVALID_OID) {
+    LOG_TRACE("Cannot find sequence %s to drop!", sequence_name.c_str());


Logging level of LOG_TRACE may be too low here. May consider switching to a higher level, such as LOG_INFO.

Throw an exception.

lym953 · 2018-04-19T00:30:23Z

src/catalog/sequence_catalog.cpp

+  // 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!


Typo: "carefull"

lym953 · 2018-04-19T00:33:23Z

src/catalog/sequence_catalog.cpp

+  // 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!


Typo: "carefull"

lym953 · 2018-04-19T00:37:48Z

src/catalog/sequence_catalog.cpp

+  }
+
+  PELOTON_ASSERT(result_tiles->size() == 1);
+  oid_t result;


Since the variable name "result" does not give any further information, how about compressing lines 293-296 into one line:
return (*result_tiles)[0]->GetValue(0, 0).GetAs<oid_t>();

lym953 · 2018-04-19T00:46:20Z

src/executor/plan_executor.cpp

@@ -181,6 +186,7 @@ int PlanExecutor::ExecutePlan(
    std::vector<std::unique_ptr<executor::LogicalTile>> &logical_tile_list) {
  PELOTON_ASSERT(plan != nullptr);
  LOG_TRACE("PlanExecutor Start with transaction");
+  LOG_DEBUG("PlanExecutor 2");


It seems you forgot to delete this line :)

lym953 · 2018-04-19T00:50:02Z

src/function/old_engine_string_functions.cpp

@@ -235,5 +235,18 @@ type::Value OldEngineStringFunctions::Lower(
  throw Exception{"Lower not implemented in old engine"};
 }

+type::Value OldEngineStringFunctions::Nextval(
+        UNUSED_ATTRIBUTE const std::vector<type::Value> &args) {


"UNUSED_ATTRIBUTE" may be removed.

lym953 · 2018-04-19T00:50:22Z

src/function/old_engine_string_functions.cpp

+}
+
+type::Value OldEngineStringFunctions::Currval(
+        UNUSED_ATTRIBUTE const std::vector<type::Value> &args) {


"UNUSED_ATTRIBUTE" may be removed.

lym953 · 2018-04-19T00:59:21Z

src/include/parser/parsenodes.h

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

+typedef struct CreateSeqStmt


Not properly formatted

lym953 · 2018-04-19T01:01:37Z

src/parser/postgresparser.cpp

+void PostgresParser::parse_sequence_params(List *options,
+                                           parser::CreateStatement *result) {
+  DefElem *start_value = NULL;
+  // DefElem    *restart_value = NULL;


apavlo · 2018-06-21T13:32:40Z

Replaced by #1407

HenryZhou0333 and others added 19 commits March 22, 2018 16:07

finish part of parser; need to add handler function in create_executo…

30d48ac

…r.cpp

finish part of parser; need to add handler function in create_executo…

086b493

…r.cpp

Merge branch 'master' of https://github.com/HenryZhou0333/peloton

59b85f6

Merge remote-tracking branch 'upstream/master'

282afb3

add postgresql parser test for sequences

28b702e

remove AS in sequencce

ddb3dd7

add exception for redefined parser

26230e3

finish compiling create sequence

b273056

Merge remote-tracking branch 'upstream/master'

d90a649

having issue with GetSequence()

c1a4e1d

add new GetSequence impl; fix BasicTest in sequence

f8ec42a

add tests for sequence

0d3521c

fix merge conflict

8fe7d6c

one step before submitting pr

87680be

change way of update pg_sequence table in nextval

7d7955c

style fix

503c89f

rename sequence test

7762354

Merge remote-tracking branch 'upstream/master'

b340c86

remove unnecessary file

6897566

apavlo added do not merge class-project labels Apr 12, 2018

danae-s and others added 2 commits April 14, 2018 17:59

adding nextval & currval functions; comment out lock and nextval not …

b8c7317

…functioning well

update pg_sequence after calling nextval; have seg fault in txn;

d665c94

yjc408 reviewed Apr 18, 2018

View reviewed changes

src/catalog/sequence_catalog.cpp

SequenceCatalog::~SequenceCatalog() {}

/* @brief Delete the sequence by name.

Copy link

yjc408 Apr 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent brief here.

yjc408 reviewed Apr 18, 2018

View reviewed changes

lym953 reviewed Apr 18, 2018

View reviewed changes

yjc408 reviewed Apr 19, 2018

View reviewed changes

yangjuns reviewed Apr 19, 2018

View reviewed changes

lym953 reviewed Apr 19, 2018

View reviewed changes

apavlo closed this Jun 21, 2018


		SequenceCatalog::~SequenceCatalog() {}

		/* @brief Delete the sequence by name.

[15721] Implement Sequence in peloton #1292

[15721] Implement Sequence in peloton #1292

Conversation

HenryZhou0333 commented Apr 12, 2018

coveralls commented Apr 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yjc408 Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangjuns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apavlo commented Jun 21, 2018

yjc408 Apr 19, 2018 •

edited

Loading